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

Open a row/link in a new tab #574

Merged
merged 9 commits into from
Apr 30, 2024
Merged

Open a row/link in a new tab #574

merged 9 commits into from
Apr 30, 2024

Conversation

erwindon
Copy link
Owner

Is your feature request related to a problem? Please describe.

Many pages like pillars, grains, jobs, etc., contain an extensive list that might take a while to load, depending on the number of minions. In this context, whenever you click in the row, it loads the content on the same page. But once you go back, it needs to load everything again.

There is a good side to always getting the latest data. But it is very annoying when you have a huge list or a filter to load, and when you get back to the page, it loads the default content again. It would be amazing to open a new tab for each job we want to look deeper into without the need to load the whole list for each of them (that would also decrease the load on the server calls).

Describe the solution you'd like

Create a configuration to force every row click to open in a new tab so the user can decide whether this behavior is more convenient. Right-clicking and opening a new tab could also be possible as a solution.

Describe alternatives you've considered

Adding an extra button on this kind of page to open the content in a new tab (if the row behavior is more challenging to implement).

Additional context

I could contribute if you gave me some guidance. I saw panels and page folders, but I'm not sure exactly how to add things there.

Thanks in advance for reading the message and maintaining this great project. Cheers!

@erwindon
Copy link
Owner

migrated to PR

@erwindon
Copy link
Owner

@rcmoutinho
I've turned this issue into a PR so that I can experiment on a branch and let that be visible for you (and others).
The first implementation tries to use the CTRL key to open the page in a new tab.
We'll see how that works out...

@rcmoutinho
Copy link
Author

Outstanding! Sounds great to me! Looking forward to seeing your tests! Thanks again!

@erwindon
Copy link
Owner

erwindon commented Apr 25, 2024

I've pushed a small implementation on the branch from this PR.

  • ALT-click now opens a new page in a new browser-tab.
    I tried CTRL-click, but that interfered too much with the "select-text" function of the browser.
    I tried adding extra menu-items but that would mean lots of extra menu items, which is too much.
    I tried adding icon on the row, but that left the menu-item without solution.
  • For this branch this ONLY works for page "Grains" at this moment:
    • For ALT-click on the table-row; and
    • For ALT-click on the (only) menu item of the menu for each row.
  • The new page has no idea that it was opened this way. But there are several possible variations:
    • The new page does not have a logo/menubar/cmdbutton on top; and/or
    • The new page does not have a close-button on the page; and/or
    • The new page does not have a "last-7-jobs" panel; and/or
    • suggestions?...

@rcmoutinho
Copy link
Author

To be honest, whatever works to open a new tab, I would be happy.

And just tested locally, and the ALT-Click works great 🎉

My opinion on what's available on the new page is just what is expected from the click. I would not add a menu or last job panel, just the information about the click (grain, pillar, job, or whatever info we are clicking). It would be a simple light call to the server to collect the precise information it needs. Once we read the page, we can just close it and continue on the context of the main page already opened.

Let me know what you think about it.
Thanks for the awesome improvement!

@erwindon
Copy link
Owner

thx for the feedback!
It may take a while before I have this complete as I now have the following tasks:

  • update all table-row (navigation)clicks to allow open in new window
  • update all popup-menu (navigation)clicks to allow open in new window
  • provide the new tab/window with additional information (in the url) to indicate that it is a new tab/window
  • hide the on-page close-button on a new tab/window
  • hide the menu bar on a new tab/window (keep the "SaltGUI"-part visible?)
  • hide the jobs panel on a new tab/window
  • test with logout on main-window, what should happen on/with secondary windows?
  • update documentation
  • think about a visual clue for this and add that too

@rcmoutinho
Copy link
Author

Damn! Indeed, it's a lot, but you have a great plan.

Would hiding info prevent sending extra calls to the server or just not showing on the UI?

I am okay with or without the SaltGUI logo/menu. The information is the most essential part. I could execute the same module call to get it, but it would defeat the UI's purpose of making it easier/quicker to get the information.

@gseguinbourgeois
Copy link

Thank you for making this possible.

This little feature makes SaltGUI become a power tool.
When launching jobs, highstates, orch on dozens of minions at a time, I think this tool should limit the noise to a maximum. (events, find_job, etc.)
And I think this here is the biggest part.

The list of tasks seem good. Good luck with the design as well!

@erwindon
Copy link
Owner

Would hiding info prevent sending extra calls to the server or just not showing on the UI?

As you have already seen, there is a separation between the pages and the panels.
When you look at the code for the pages, you'll see that it is just the code to create the desired panels and add them.
That code will be updated to only create+add the left-most panel and not create+add the jobs panel.
Api-calls will only take place for the created+added panels.

@rcmoutinho
Copy link
Author

Perfect! Thanks for the extra details about the process.

@erwindon erwindon marked this pull request as draft April 27, 2024 00:41
@erwindon
Copy link
Owner

erwindon commented Apr 27, 2024

progress tracker:

  • update all table-row (navigation)clicks to allow open in new window
  • update all popup-menu (navigation)clicks to allow open in new window
  • provide the new tab/window with additional information (in the url) to indicate that it is a new tab/window
  • hide the on-page close-button on a new tab/window
  • hide the menu bar on a new tab/window (keep the "SaltGUI"-part visible?)
  • hide the jobs panel on a new tab/window
  • test with logout on main-window, what should happen on/with secondary windows?
  • update documentation
  • prevent focus-border after CTRL-click on TR
  • fix tooltips at top of window that (with the header removal) fall off the top of the window
  • think about a visual clue for this and add that too (solved by creating a new issue: add a navigation indicator for table rows #575)

note: often git push --force is used on this branch, use git pull -r to update your workspace

Repository owner deleted a comment from sonarqubecloud bot Apr 27, 2024
@erwindon erwindon marked this pull request as ready for review April 27, 2024 09:22
@erwindon erwindon marked this pull request as draft April 27, 2024 09:23
@erwindon
Copy link
Owner

I tried CTRL-click, but that interfered too much with the "select-text" function of the browser.

Enabled it anyway.
This has the fun-behavior that CTRL-click opens in a new tab without making that the current tab and ALT-click opens in a new tab which becomes the current tab. Note that that happens automatic, without specific instructions in SaltGUI. So it may behave different in different browsers. But at least Firefox+Chrome+Edge do this.

@rcmoutinho
Copy link
Author

Interesting! I remember that once I clicked it, it opened and activated the new tab. However, I think the default behavior on the browser for Alt is to "save link as". I will test all your changes first thing Monday morning and give you extra feedback if there is any.

Thanks again for the massive code changes to make it happen 🎉

@erwindon erwindon force-pushed the new-window branch 3 times, most recently from 13c4641 to 0ca6ee3 Compare April 27, 2024 18:16
@erwindon
Copy link
Owner

fix tooltips at top of window that (with the header removal) fall off the top of the window

solution is to show the header again, only the menu-items are now hidden in secondary tab/page.

@rcmoutinho
Copy link
Author

This is beautiful! Opening five job results simultaneously using CTRL is even better than ALT (considering it changes the page focus). Both approaches are working on my side 🎉

I enabled all notifications to ensure I update them as soon as possible once this is merged with the master.
Thanks a lot again! 🤩

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
12.1% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

@erwindon erwindon marked this pull request as ready for review April 29, 2024 21:14
@erwindon
Copy link
Owner

erwindon commented Apr 29, 2024

@rcmoutinho
I've handled the last two tasks.
The visualization of this all is delegated to a separate issue #575.
I found how to undo the table-cell-selection that happens on a ctrl-click in a table.
This concludes the coding part of this issue/PR.

I plan to make a new release of SaltGUI after this issue and issue #571 are done.

Can you give this PR one final round of testing?

@erwindon erwindon assigned rcmoutinho and unassigned erwindon Apr 29, 2024
@rcmoutinho
Copy link
Author

@erwindon re-deployed the branch, and it continues to work great! Thanks a lot for all the work!

@erwindon erwindon merged commit 0202543 into master Apr 30, 2024
3 of 4 checks passed
@erwindon erwindon deleted the new-window branch April 30, 2024 18:33
@erwindon erwindon assigned erwindon and unassigned rcmoutinho May 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants