-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Move tenant buttons into dropdown menu #12028
Conversation
Seems there's some changes need to be done in the functional tests |
You will need to add a click trigger on the dropdown menu before the click on the setup button. |
I might look to this test later, but again we need a way to make these tests pass or fail for both OS. What I'm see there's some inconsistency it passes in Windows and fails in Linux |
The functional tests are not executed under Windows that's why. |
<a asp-action="Reload" asp-route-id="@entry.Name" class="btn btn-secondary btn-sm reload" data-url-af="UnsafeUrl">@T["Reload"]</a> | ||
<div class="dropdown float-end"> | ||
<a id="tenantDropdownMenuButton" class="btn text-secondary" data-bs-toggle="dropdown" aria-expanded="false"><i class="fa fa-ellipsis-v"></i></a> | ||
<ul class="dropdown-menu" aria-labelledby="tenantDropdownMenuButton"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aria-labelledby Ref doesn’t have any text
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't understand
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-labelledby
Here it refers to button but button doesn’t have accessible text
Not sure it's a good thing, as it is the buttons are not only action buttons but also states info, so we can see clearly the tenant states and what we can do. |
The states are clearly shown as a badge. The current UI change:
FYI such menu actions already shown in GitHub & AzureDevOps for better UX. May be the issue we are looking from developer point of you ;) |
Ok, we need to make sure we are consistent here. While I don't dislike entirely the UI change; here we need to take into consideration that the Content Items list uses an "Action" button for these. This is why I recommended using the same UI concept. |
The inline action buttons are simply icons here on Github. |
@hishamco thanks for putting together this PR. Can you please fix the conflict? I personally do not like hiding all the buttons for all actions. Very common functions like "Edit" should be visible all the time. I don't want to have to make 2 click to perform a common functions. The idea here is improve the UI not complicating it like devops :). I am also not a big fan of DevOps UI as @ns8482e mentioned. I think in this UI, we should show "Edit" and "View" buttons, then move the "Reload", "Enable", "Disable", "Features", and "Remove" button should be in the "Actions" button. |
Sure
May be but the issue is everyone of us has it's own common functionality ;) |
Let me check this with a UI expert and see what I can come up with |
This pull request has merge conflicts. Please resolve those before requesting a review. |
Putting the buttons under a nondescript three-dot menu is a hard no from me. |
... is a hard no for me to. I am okay with "Actions" drop down button if that is an option |
It's much better for UI clarity |
How does hiding buttons clarify intent? |
From UI/UX perspective when there are many buttons like what we have, it would be nice to introduce an action menu instead |
This is how the current UI looks like if all the possible buttons are displayed, in a common full HD resolution: Ultimately, design is subjective until we do large sample size research for this with focus groups (which would probably be excessive for this) but this doesn't seem to be too many buttons for me. Nor there is any lack of screen real estate that we'd use better. Same if you have 10 tenants on a page. Did you encounter somebody complaining about this UI? |
While I saw many web framework applications, & CMSs I never see something like this :) Also the coloring is another story lol |
Do you want to bring this up at the meeting or something so we can get a closure? Here we have 2-3 against votes, and other remarks that necessitate changes even if the basic direction is agreed on. |
Probably yes, also I might demo the Culture Picker in localization part #13354 |
# Conflicts: # src/OrchardCore.Modules/OrchardCore.Tenants/Views/Admin/Index.cshtml
I'm OK with that. |
Sure everyone will like and dislike some of the UI, but I was using things from UI/UX perspective |
It seems that this pull request didn't really move for quite a while. Is this something you'd like to revisit any time soon or should we close? Please comment if you'd like to pick it up and remove the "stale" label. |
I will close this while there's not much interest in UI improvements, so I will move this to Orchard Core Contrib (OCC), while I'm already some improvements to the admin UI |
Address a part of #12006
DropDown.Menu.mp4