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

DS implementation - SideNav #13063

Closed
wants to merge 7 commits into from
Closed

DS implementation - SideNav #13063

wants to merge 7 commits into from

Conversation

pettinarip
Copy link
Member

@pettinarip pettinarip commented May 28, 2024

@github-actions github-actions bot added the tooling 🔧 Changes related to tooling of the project label May 28, 2024
Copy link

netlify bot commented May 28, 2024

Deploy Preview for ethereumorg ready!

Name Link
🔨 Latest commit 92276bb
🔍 Latest deploy log https://app.netlify.com/sites/ethereumorg/deploys/667082617506a20008baea6d
😎 Deploy Preview https://deploy-preview-13063--ethereumorg.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
7 paths audited
Performance: 42 (🟢 up 4 from production)
Accessibility: 92 (no change from production)
Best Practices: 83 (🔴 down 9 from production)
SEO: 93 (no change from production)
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

@pettinarip pettinarip changed the title DS SideNav DS implementation - SideNav May 31, 2024
@nloureiro
Copy link
Contributor

The open and closing work differently on some items. screen cast done on brave

Screen.Recording.2024-06-12.at.12.23.57.mov

@pettinarip
Copy link
Member Author

The open and closing work differently on some items. screen cast done on brave

Screen.Recording.2024-06-12.at.12.23.57.mov

So, you are right, we have 2 types of expandable items:

  1. The "expandable item"
  2. The "expandable link item"

The first one, you can click the whole row and it will expand the content.
The second one, depends on where you click:

  • If you click on the text (link) it will navigate to that page or doc
  • If you click near the arrow, it will expand

@nloureiro
Copy link
Contributor

The open and closing work differently on some items. screen cast done on brave
Screen.Recording.2024-06-12.at.12.23.57.mov

So, you are right, we have 2 types of expandable items:

  1. The "expandable item"
  2. The "expandable link item"

The first one, you can click the whole row and it will expand the content. The second one, depends on where you click:

  • If you click on the text (link) it will navigate to that page or doc
  • If you click near the arrow, it will expand

This feels weird a bit like a bug.

If we need to have 2 interactions on the same button, we need to be clearer about what does what.
In production, I don't see this (on the left PR on the right production)

Screen.Recording.2024-06-12.at.15.17.05.mov

here we open the link and open the accordion always.

is it possible to change this? or it's a big hack. We might as well just change the component.

@pettinarip
Copy link
Member Author

@nloureiro correct. I get it now. It works the first time you click on it because it navigates to that page and then expands the content. But when you collapse and try to expand it again by clicking on the text, it doesn't expand.

Yeah, I think we can do that, shouldn't be a problem.

@wackerow
Copy link
Member

Can we default to these being open? Seeing an animation on page load where all of these nav accordions start collapsed and then animate open.

@wackerow
Copy link
Member

image

Also noting on RTL langs, the items are mixing right and left alignment (in this screenshot these should all be right aligned).

@TylerAPfledderer
Copy link
Contributor

Can we default to these being open? Seeing an animation on page load where all of these nav accordions start collapsed and then animate open.

defaultIndex should be the prop to use here for this purpose. the index prop is meant to be used in controlled state, which this accordion set should not be controlled.

However, this does not resolve the delayed expanded, which I believe is coming from above the accordion. If you test the Chakra Accordion component in static isolation, defaultValue should have the expanded item render on load and not be lazy about it.

@pettinarip
Copy link
Member Author

Can we default to these being open? Seeing an animation on page load where all of these nav accordions start collapsed and then animate open.

defaultIndex should be the prop to use here for this purpose. the index prop is meant to be used in controlled state, which this accordion set should not be controlled.

However, this does not resolve the delayed expanded, which I believe is coming from above the accordion. If you test the Chakra Accordion component in static isolation, defaultValue should have the expanded item render on load and not be lazy about it.

The reason I'm using the controlled version is because we need to open it manually when internal navigation happens. When you click on an element that is also a link, we want to navigate to that doc page and also want to expand the content. You can see the useEffect that controls this part.

Regarding the delayed expanded, I tried mounting an empty Accordion on an empty test page (and even hardcoding index={0}) and the same thing happened. I'm not sure if this is how the Accordion is supposed to work at this point.

In SB I can see this on mounting:

demo.mp4

Seems that the top level items are expanded correctly but the nested one is expanded in the next render...

Also noting on RTL langs, the items are mixing right and left alignment (in this screenshot these should all be right aligned).

Nice catch, fixed!

@wackerow
Copy link
Member

Initial load bug

Functionally this seems to be working really well, but still seeing the bug s described above where the nested menus are animating open.

Screen.Recording.2024-06-18.at.12.24.31.mov

Will highlight a frame when I did the first refresh above, showing the SideNav initially collapsed, and the code blocks rendering in the wrong color mode:

image

Hover styles

  • Menu dropdown triggers: These are button elements, and the entire box has hover effects, and clickable 🎉
  • Menu item links: These are each a div element wrapping an a element. The div has a hover effect independent of the a link effect, creating a zone around the perimeter where the box hover effect occurs, but it's not yet clickable 🐛.

Would consider flipping so the a wraps the div, or use ButtonLink component.

@pettinarip
Copy link
Member Author

@wackerow thanks for the detailed review and snapshots. For now, as I said, I don't really know if we can show them as expanded by default.

I'll keep debugging and testing to see if there's anything missing, but I'd say: if we can't fix this, or the accordion can't be rendered as expanded by default, we can revert and use the previous implementation + the DS styles.

I will consider this issue a blocker. We should be able to render this list with an open state from the server side.

=====
Nice observation on the button vs link items, will test your suggestion and see how it goes.

@pettinarip
Copy link
Member Author

@TylerAPfledderer I've tested the Accordion on an empty project and can confirm that the Accordion only expands on the client side (you can clearly see this by disabling js in your browser). It seems that we can't render the expanded version on the server side.

We might have to revert the Accordion at least on this component. LMK if you have any other ideas or if you think there is another way to do this.

Copy link
Contributor

@TylerAPfledderer TylerAPfledderer left a comment

Choose a reason for hiding this comment

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

@TylerAPfledderer I've tested the Accordion on an empty project and can confirm that the Accordion only expands on the client side (you can clearly see this by disabling js in your browser). It seems that we can't render the expanded version on the server side.
We might have to revert the Accordion at least on this component. LMK if you have any other ideas or if you think there is another way to do this.

I made an inquiry with the Chakra team to find out if this is expected behavior that is being addressed in Chakra v3. I'm not quite familiar as to why this couldn't be done server-side. Also inquired if there is a workaround to be offered or if the component in the v2 package can be patched.

@TylerAPfledderer
Copy link
Contributor

@pettinarip after checking with some Chakra folks, this is probably a bug within Accordion.

But at the moment, I'll need to do some internal testing to confirm.

Again, we do not see this to be an issue when switching to v3

@pettinarip
Copy link
Member Author

@TylerAPfledderer thanks! ok, good to know. I'll revert the Accordion implementation for now and keep using the previous implementation but using the new DS styles.

@TylerAPfledderer
Copy link
Contributor

@TylerAPfledderer thanks! ok, good to know. I'll revert the Accordion implementation for now and keep using the previous implementation but using the new DS styles.

@pettinarip sounds good. I'll get back to you either in this PR or discord if I have any update on what I find.

@pettinarip
Copy link
Member Author

Closing in favor of #13230

@pettinarip pettinarip closed this Jun 24, 2024
@github-actions github-actions bot added the abandoned This has been abandoned or will not be implemented label Jun 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
abandoned This has been abandoned or will not be implemented tooling 🔧 Changes related to tooling of the project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SideNav
4 participants