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

Update sidebar #16017

Closed
wants to merge 28 commits into from
Closed

Update sidebar #16017

wants to merge 28 commits into from

Conversation

KTibow
Copy link
Contributor

@KTibow KTibow commented Apr 2, 2023

Proposed change

Updates the sidebar to

  • use raw links instead of paper-listbox, fixes some ugly apperance stuff and ctrl+clicking
  • use m3 styling
  • refactor so it isn't a megacomponent

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (thank you!)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example configuration

NA

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue or discussion:
  • Link to documentation pull request:

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

@KTibow KTibow marked this pull request as ready for review April 2, 2023 14:58
@KTibow
Copy link
Contributor Author

KTibow commented Apr 2, 2023

Original M3 Example Implementation
image image image
image image image

@bramkragten
Copy link
Member

use raw links instead of paper-listbox, fixes some ugly apperance stuff and ctrl+clicking

With the list element removed, we now no longer have keyboard navigation, like typeahead and simple arrow handling. Also is it now missing a lot of a11y feats.

@KTibow
Copy link
Contributor Author

KTibow commented Apr 2, 2023

re: a11y, after checking on my machine:

  • what is "typeahead and simple arrow handling"?
  • keyboard navigation still works with shift-tab/tab and enter

@LasseRosenow
Copy link
Contributor

Maybe better to just wait for the @material/web drawer component?

@bramkragten
Copy link
Member

bramkragten commented Apr 6, 2023

re: a11y, after checking on my machine:

  • what is "typeahead and simple arrow handling"?
  • keyboard navigation still works with shift-tab/tab and enter

Press a letter when focus is on the old sidebar list, it will focus the item starting with the letter you pressed.
Also a list should not (only) use tab, but key up down according to a11y specs

@KTibow
Copy link
Contributor Author

KTibow commented Apr 6, 2023

@LasseRosenow there is a nav drawer component but that just wraps whatever content you want to put inside of it - could be they'll add more later as it's marked as wip
@bramkragten what a11y specs are you referring to

@bramkragten
Copy link
Member

Basic a11y features for lists https://w3c.github.io/aria/#list

@KTibow
Copy link
Contributor Author

KTibow commented Apr 21, 2023

Well I'm not sure how to move forward on this, as using paper-listbox would reintroduce the bugs like ctrl+clicking behavior. Should I make a custom implementation to handle those keys?

@bramkragten
Copy link
Member

Well I'm not sure how to move forward on this, as using paper-listbox would reintroduce the bugs like ctrl+clicking behavior. Should I make a custom implementation to handle those keys?

You can use mwc-list but that will not work with anchor links, so open in new tab etc will no longer work.

- fix currentpanel logic
- disable native outline, indicate focus on active page
- prevent focus on container
- space works to activate an item
- typing and up/down works to focus an item
@KTibow
Copy link
Contributor Author

KTibow commented Jun 3, 2023

I believe this PR should be ready for comment now. In my testing I didn't find any bugs, and everything from the ARIA listbox guidelines (if it was in there previously) is now working.
(The only thing I can think of changing is increasing the touch targets)

@silamon
Copy link
Contributor

silamon commented Aug 2, 2023

Too bad this didn't got a review yet, I've had a go and tested your changes. I didn't look at the code yet.

The sidebar is so much smoother by removing that paper-listbox. It feels like the dashboard also loads faster.
I hope you don't mind if I share some things that didn't work for me:

  • Reordering the dashboards: I can go in edit mode, but dragging dashboards to other places doesn't seem to move it.
  • The scrollbar takes a lot of place as can be seen in the screenshot
    image

@KTibow
Copy link
Contributor Author

KTibow commented Aug 2, 2023

i'll see if i can reproduce that

Comment on lines +138 to +141
static shadowRootOptions = {
...LitElement.shadowRootOptions,
delegatesFocus: true,
};
Copy link
Contributor

@silamon silamon Aug 8, 2023

Choose a reason for hiding this comment

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

This is the reason the ordering of dashboards no longer work. I'm not sure how to solve it though, unless it is accepted that focus can be on the container, which I assume this is the reason the code exists.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can't reproduce the issue with reordering dashboards
Can you send a screen recording and other relevant info

Copy link
Contributor

Choose a reason for hiding this comment

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

So, I can reproduce this on both Microsoft Edge and Google Chrome latest version. On Mozilla Firefox it works good. I'll see if I can create a screen recording.

Copy link
Contributor

@silamon silamon Oct 19, 2023

Choose a reason for hiding this comment

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

I've retested today and I'm still unable to reorder the dashboards on Chrome v118 though well done on this PR so far.

Here's a screen recording:
sidebar

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can't reproduce w/ Chromium 117.0.5938.132 or Chrome Mobile 118.0.5993.80
Does it work if you clear your localstorage by chance

Copy link
Contributor

Choose a reason for hiding this comment

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

Too bad, removing localstorage doesn't solve it. It works if the shadowRootOptions are removed or delegatesFocus is set to false. Let's wait until somebody other reviews.

@bramkragten
Copy link
Member

Major UI changes like these will have to be approved by our design department, CC @matthiasdebaat

@bramkragten bramkragten added the needs design preview PRs with this label will trigger a GitHub action to generate a gallery preview label Aug 17, 2023
@KTibow
Copy link
Contributor Author

KTibow commented Aug 17, 2023

I see that there was a "needs design preview" label added. Assuming that refers to https://design.home-assistant.io/, I have some questions about how to implement one:

  • How exactly do I display the sidebar? Am I supposed to display the whole sidebar or individual panels? If I need to display the whole sidebar:
    • What kinds of states do I need to display? Do I need to show what it looks like with devtools, on mobile, etc? If so, should I just display all the different kinds of sidebars, or provide switches that can be used?
    • Where can I learn about how to set up a fake hass?
  • As someone who isn't as familiar with the frontend as someone in the core team might be, is there a specific contributing guide for how to create a design preview?
  • Also, is there any specific reason a design preview was requested? The previous sidebar isn't there. Is this a norm for new UI components? Does this make it easier to review? As you can tell I'm not sure if I know enough to create a design preview.

@KTibow
Copy link
Contributor Author

KTibow commented Sep 21, 2023

Hey, I'd just like to recap that with 3f1fe22 and 586e12f, I added/fixed these things:

  • A reduced item height
  • Using the full badge when expanded
  • A wiggling animation when in edit mode
  • Full touch targets

If the code looks all good, this PR is ready to merge.

@silamon
Copy link
Contributor

silamon commented Oct 19, 2023

I've retested today since I hope this one can be merged soon.

I've found a little issue in the editing mode for rtl languages.

ltr vs rtl:
image
image

@KTibow
Copy link
Contributor Author

KTibow commented Oct 21, 2023

Should be resolved on the latest commit

@silamon
Copy link
Contributor

silamon commented Nov 2, 2023

@bramkragten can this get a review somehow or is there anything that still needs to be done before review?

@bramkragten
Copy link
Member

I don't like the text color and especially the icon color of the sidebar, is there a reason these are now so dark?
Also the text "Home Assistant" on top of the expanded sidebar looks very small?
I also miss things like the ripple effect in your implementation, but that might be ok.

@KTibow
Copy link
Contributor Author

KTibow commented Dec 5, 2023

I don't like the text color and especially the icon color of the sidebar, is there a reason these are now so dark?

In the dark theme or light theme? Thing is that M3 calls for on-surface-variant, but we only have primary-text-color and secondary-text-color (a much less visible color). While I could decrease the opacity (to 90%) to match on-surface-variant, since I use rgba and var in my code and you can't multiply rgba, this wouldn't work.

Also the text "Home Assistant" on top of the expanded sidebar looks very small?

Yup, this is in compliance with Material 3. The title should be title-small, which is 14pt. Technically speaking it should be even smaller as Home Assistant uses a text size that is 7/8 the normal size.

I also miss things like the ripple effect in your implementation, but that might be ok.

The previous implementation didn't have ripples, and I'm not exactly sure which ripples to use.

Copy link

github-actions bot commented Mar 4, 2024

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

@github-actions github-actions bot added the stale label Mar 4, 2024
@codyc1515
Copy link
Contributor

Current or can be closed?

@github-actions github-actions bot removed the stale label Mar 4, 2024
@KTibow
Copy link
Contributor Author

KTibow commented Mar 5, 2024

Let me fix the conflicts (yet again..)

@KTibow
Copy link
Contributor Author

KTibow commented Mar 5, 2024

Actually there have been too many changes for me to want to bother with fixing this yet again. I'm just sad that this never got a chance to get merged.

@KTibow KTibow closed this Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed needs design preview PRs with this label will trigger a GitHub action to generate a gallery preview
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants