Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Group Mullvad Nodes by Country, Scrollable Nodes Section, Exit node subtitle #18

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

rjocoleman
Copy link
Contributor

More changes in a single commit than I'd like, sorry!

This PR:

  • creates a ScrollablePopupMenu class that supports scrolling and sections with titles.
  • Adds nodes in a PopupMenu.PopupMenuSection to it.
  • If Mullvad nodes are present these are added as a section to the scrollable menu too.
  • in tailscale.js adds the node location
  • Groups Mullvad nodes by country (if more than one node exists in that country)
  • Creates a collapsable menu per country
  • Puts the mullvad exit node at the top of the mullvad nodes list
  • In tailscale.js adds the exit node name (if set) to the client
  • Displays the exit node name as a subtitle on the QuickSettings toggle.

@joaophi
Copy link
Owner

joaophi commented Nov 28, 2023

Hi @rjocoleman,

Thanks for you contribution!!

I have a few points I'd like you take a look at:

  • A lot of empty spaces with few nodes
  • Can you disable the hover effect for the whole block, and enabled it for the devices only
  • Keyboard support
    image

You addition to show the exitnode as subtitle I already merged to master a5ce530

I also created a PR(#19) to fix this scroll issue, it's more simple as it simply forces the scrollbar to show in the submenu, so if the items above are hard to achieve we can use as fallback.

@rjocoleman
Copy link
Contributor Author

@joaophi I've addessed some of these issues with a new PR on #20

I think its slightly more idiomatic gnome-shell than this PR.

The height of the box is now set dynamically.
The hover styling is a bit better, but due to the way they're nested I couldn't work out the combination to get it quite right.

Keyboard support is problematic. I ended up totally reinventing scrolling a couple of ways and it seems very fragile, so I removed the code. In #20 there is some interaction between scrolling when the mouse is over an open sub menu. I didn't track down that issue yet either.
Both cases, I think there is a more idiomatic way to do some of these things that will lead to less hacking/reimplimentation of primitives but i didn't see it yet.

I have also updated this PR with many of features, but I don't think the approach here is as good as #20, but there are some trade-offs for both.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants