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

feat(UserMenu): add target property to usermenu component #41659

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

Conversation

fitrahmunir
Copy link

Summary

Add target property to UserMenu and UserMenuEntry components and add target "_blank" to Administration settings

TODO

  • Add target property to UserMenu and UserMenuEntry components
  • Add target "_blank" to admin_settings

Checklist

@fitrahmunir fitrahmunir force-pushed the feat/add-target-property-to-usermenu-component branch 5 times, most recently from 4d9756b to fb009e9 Compare November 23, 2023 09:08
@nickvergessen
Copy link
Member

Failing CI is related:
image

@nickvergessen
Copy link
Member

I don't think the admin settings should open in a new tab all the time?
Neither the user menu links...

cc @nextcloud/designers

@blizzz blizzz added this to the Nextcloud 29 milestone Nov 23, 2023
@blizzz blizzz added the 3. to review Waiting for reviews label Nov 23, 2023
@fitrahmunir fitrahmunir force-pushed the feat/add-target-property-to-usermenu-component branch from fb009e9 to c9d4e40 Compare November 23, 2023 23:01
@marcoambrosini
Copy link
Member

This is an old one.. I agree with the proposed change. I think that admin settings and personal settings, and all apps for that matter, should open in another tab. This solution:

  • Reduces friction: once I'm done with the admin settings work, I don't have to remember where I was and what I was doing. I just close the tab and the first thing I see is my work exactly where I left it.
    Moreover, had open settings in the same tab, the state of the app that I navigated away from is not saved, and will likely never be saved:
    It has been suggested in the past that we could "remember" the state of each app, but this is hard to implement and maybe not even desirable in most cases. In any event, we'd be reinventing something that's already there at the browser level: multitasking. So in my opinion it's not a wise allocation of resources.

  • Cuts on lading times: I don't have to load the app that I was using again.

So by insisting on loading different things in the same tab in pursuit of the big single page application feel, we're effectively discouraging users from using the multitasking feature that comes with a web browser.

@nextcloud/designers please go and take a look at other (multi-app) projects, the usual suspects. All of them open apps and global settings in new tabs, despite the fact that their loading times are in general faster than ours. (Testing their loading times vs. my local docker loading times).

@szaimen
Copy link
Contributor

szaimen commented Nov 30, 2023

Hm... I think I don't like this idea. New tabs usually should only be opened for links to external services or if it is really necessary IIRC...

@marcoambrosini
Copy link
Member

New tabs usually should only be opened for links to external services

That is just a convention that seems to be born out of not wanting users to navigate away from your site. Not a UX rule.

As per the necessity of it, I think here it's warranted because of the reasons I mentioned above. And our friends at G & M, that have all the UX research resources that they want, reached that conclusion too.

@szaimen
Copy link
Contributor

szaimen commented Dec 1, 2023

And our friends at G & M, that have all the UX research resources that they want, reached that conclusion too.

Fine by me then 👍

@fitrahmunir fitrahmunir force-pushed the feat/add-target-property-to-usermenu-component branch from c9d4e40 to c7a9d02 Compare December 1, 2023 23:26
@fitrahmunir fitrahmunir force-pushed the feat/add-target-property-to-usermenu-component branch 3 times, most recently from 6ad0f1b to 5962797 Compare February 11, 2024 15:04
@Altahrim Altahrim mentioned this pull request Mar 12, 2024
@fitrahmunir fitrahmunir force-pushed the feat/add-target-property-to-usermenu-component branch from 5962797 to 7f0aed1 Compare March 12, 2024 16:44
@fitrahmunir
Copy link
Author

@nickvergessen please review this PR

@fitrahmunir fitrahmunir force-pushed the feat/add-target-property-to-usermenu-component branch 8 times, most recently from ebc2167 to ce22dcc Compare March 12, 2024 19:02
@fitrahmunir fitrahmunir force-pushed the feat/add-target-property-to-usermenu-component branch from ce22dcc to 5790752 Compare March 12, 2024 19:36
@nickvergessen nickvergessen requested review from a team, Pytal, szaimen and emoral435 and removed request for a team March 14, 2024 08:21
@Altahrim Altahrim mentioned this pull request Mar 14, 2024
Copy link
Contributor

@emoral435 emoral435 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the designers, I think that not having the user exit out of the page is always convenient. IMO - I would rather use more RAM for the tab than have to navigate back to my page xD But I like the code changes as well, just a minor request

Comment on lines +75 to +78
target: {
type: String,
required: false,
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for here, can we have a default of null? In case the user does not input anything into the component, having the null would make it would not be a breaking change :)

This was referenced Mar 18, 2024
@skjnldsv skjnldsv mentioned this pull request Mar 28, 2024
81 tasks
This was referenced Apr 4, 2024
@blizzz blizzz modified the milestones: Nextcloud 29, Nextcloud 30 Apr 8, 2024
@skjnldsv skjnldsv added 2. developing Work in progress stale Ticket or PR with no recent activity and removed 3. to review Waiting for reviews labels Jul 27, 2024
This was referenced Jul 30, 2024
This was referenced Aug 5, 2024
@skjnldsv skjnldsv mentioned this pull request Aug 13, 2024
@skjnldsv skjnldsv removed this from the Nextcloud 30 milestone Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress stale Ticket or PR with no recent activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Please add 'target = "_blank"' to the link 'List of invalid files' in Settings > Admin > Overview
7 participants