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

Improve items list component #4635

Closed
gtsiolis opened this issue Jun 28, 2021 · 23 comments · Fixed by #16151, #16258 or #16657
Closed

Improve items list component #4635

gtsiolis opened this issue Jun 28, 2021 · 23 comments · Fixed by #16151, #16258 or #16657
Assignees
Labels
component: dashboard meta: never-stale This issue can never become stale team: webapp Issue belongs to the WebApp team type: improvement Improves an existing feature or existing code user experience

Comments

@gtsiolis
Copy link
Contributor

gtsiolis commented Jun 28, 2021

Problem to solve

In #4454, we've added a reusable items list component used in Workspaces, Environement Variables, etc. In parallel, we've also fixed the issue with multiple dropdowns, see #4502. Cc @corneliusludmann

However, there's still room for improving the UX and interaction of the items list component and the more actions dropdown.

Proposal

The items list component could be improved by doing the following:

  1. Do not remove the hover state on the more actions button when triggered and moving the cursor away. (Dropdown B)
  2. Do not remove the hover state of the list item when triggering the more actions and moving away the cursor. (Dropdown C)
Dropdown A Dropdown B Dropdown C
list-item-1 list-item-2 list-item-3
@gtsiolis gtsiolis added user experience component: dashboard type: improvement Improves an existing feature or existing code labels Jun 28, 2021
@stale
Copy link

stale bot commented Sep 26, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the meta: stale This issue/PR is stale and will be closed soon label Sep 26, 2021
@gtsiolis
Copy link
Contributor Author

This is still relevant. Adding meta: never-stale label.

@stale stale bot removed the meta: stale This issue/PR is stale and will be closed soon label Sep 27, 2021
@gtsiolis gtsiolis added meta: never-stale This issue can never become stale meta: stale This issue/PR is stale and will be closed soon labels Sep 27, 2021
@stale stale bot removed the meta: stale This issue/PR is stale and will be closed soon label Sep 27, 2021
@gtsiolis gtsiolis added the team: webapp Issue belongs to the WebApp team label Dec 14, 2021
@gtsiolis
Copy link
Contributor Author

Cc @jldec in case we could prioritize this small fix.

Also adding this as a good-first-issue in case someone from the community would be interested in picking this up.

@jldec
Copy link
Contributor

jldec commented Dec 15, 2021

thanks @gtsiolis

@aboqasem
Copy link

aboqasem commented Jun 10, 2022

Hi!

I was looking at the issue and noticed that the list is not consistent with keyboard tabs, it uses divs instead of buttons which does not respect semantic HTML. Maybe create another issue for accessibity?

I thought of using a group-focus, but the actions button is a div as well 😅.

@gtsiolis
Copy link
Contributor Author

Hi @aboqasem! Your comment above comes with valid points and concerns regarding accessibility. ✔️

However, this is indeed a different issue that needs to be resolved regardless what HTML elements are used under the hood and is not related with the interaction issue described in this issue.

I've opened a separate issue for improving the more actions button accessibility in the context menu component, as we generally tend to work with smaller scope issues containing MVC (minimum viable changes) so that we can ship faster and plan better. Tackling these two issues in one step or separately is completely fine. 💯

Re-posting from #10586 to also help with answering the question above.

Generally, a div-based button can become quite accessible, however, you'd still need to make the element focusable and define event handlers when a button-based button automatically takes care of this. See relevant documentation[1][2][3][...].

For example, in this case where we're using a div-based button, we're missing the basic role="button" attribute, which could improve the existing situation. ➿

@henit-chobisa
Copy link
Contributor

henit-chobisa commented Jan 27, 2023

Hey @gtsiolis, I am keen to work on this issue 🍊

@gtsiolis
Copy link
Contributor Author

Hey @henit-chobisa! Feel free to open a pull request and link back this issue. The proposal in the issue description should clearly define what we need to change here. Let me know if this is not clear enough. 🏓

@henit-chobisa
Copy link
Contributor

Thanks for getting back to me @gtsiolis, I am clear with the issue, to confirm we need to maintain the selection of the workspace item and the button corresponding to context menu till the time the drop down is active.
I have already started working on it, will soon make a PR.

@gtsiolis
Copy link
Contributor Author

Correct, @henit-chobisa!

@henit-chobisa
Copy link
Contributor

🤩, would be awesome if you could assign me, will make a PR soon.

@gtsiolis
Copy link
Contributor Author

Sure, @henit-chobisa! I generally avoid assigning anyone outside the team to avoid blocking community contributions to stale issues that never get a PR which is very common across open source projects. However, I like challenging this approach—assigned!

@henit-chobisa
Copy link
Contributor

Thank you so much for assigning me @gtsiolis, grateful

@henit-chobisa
Copy link
Contributor

@gtsiolis
I have almost solved the issue, currently it looks like this, but for this I had to pass a function all the way from WorkspaceEntry component to ContextMenu component, because the expansion state is of the ContextMenu component itself, can I improve it in some way? Like setting up context for this particular element ?

Screen.Recording.2023-01-31.at.3.07.05.PM.mov

@gtsiolis
Copy link
Contributor Author

@henit-chobisa UX changes LGTM! I don't have strong feelings on the technical side, I'd suggest opening a PR with this and let someone from the @gitpod-io/engineering-webapp chime in with code suggestions or changes. 🏓

@henit-chobisa
Copy link
Contributor

Sure @gtsiolis, I do the same,
Also, the same item component that is used to represent workspaces here, is also used to represent environment_variable list and others, so should I change all of those or I make the PR with only the workspace change ?

@gtsiolis
Copy link
Contributor Author

@henit-chobisa I'd suggest keeping the code changes to the minimum as a good MVC, so we can review and merge faster. If I recall correctly the same component should be used in other pages.

@henit-chobisa
Copy link
Contributor

Got it @gtsiolis,
I will make the PR before EOD.Thank you so much 🙏

@henit-chobisa
Copy link
Contributor

Hey @gtsiolis thank you so much for your support,
I have made the PR, which fixes the Item List components for the workspace page, would make for other sections like environment variables and Integrations too, once the current PR method gets approved.

@gtsiolis
Copy link
Contributor Author

gtsiolis commented Feb 1, 2023

I have made the PR

🙌

Can I tackle any issues from #12033 ?

@henit-chobisa The workspace list improvements are still in discussion. I'll post some updates later today or this week and ping you if you're up for making any changes. 🏓

@henit-chobisa
Copy link
Contributor

Would be really really @gtsiolis , if I can have the opportunity to work on it. 🐳

@RayAsh37
Copy link
Contributor

hey, @gtsiolis was going through the issue to understand #16420 when I came across this bug. The previous active workspace is never deactivated. I believe this isn't the intended behavior as confirmed here.

to confirm we need to maintain the selection of the workspace item and the button corresponding to context menu till the time the drop down is active. I have already started working on it, will soon make a PR.

Workspaces.Gitpod.mp4

@gtsiolis
Copy link
Contributor Author

Nice catch, @RayAsh37! ⚾

Could you open a follow-up issue for this bug?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: dashboard meta: never-stale This issue can never become stale team: webapp Issue belongs to the WebApp team type: improvement Improves an existing feature or existing code user experience
Projects
Status: In Validation
5 participants