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

[docs][site] Fixes small screen nav #18546

Merged
merged 7 commits into from
Jul 12, 2024
Merged

Conversation

ronny-mysten
Copy link
Contributor

@ronny-mysten ronny-mysten commented Jul 8, 2024

Description

Fixes broken nav on small screens. Jira DOCS-383 for more info. Also applies tailwind to some styles, which should eventually be used exclusively.

docs/site/src/components/GetStartedLink/index.tsx: Removed the logic that controls the display of the Get Started link on the home page to its own component. This should make changing the location the link navigates to easier and to change the logic behind visible/hidden and other updates. Should make updates easier because only need to change the component reference of the docusaurus component instead of the related logic.

docs/site/src/components/ThemeToggle/index.tsx: Removed the logic for showing or hiding the default theme switcher from the default docusaurus component. Moved to its own component to better support docusaurus updates in the future. Also makes replacing the default switcher in the future easier.

deleted components from theme/Navbar: These components were swizzled from docusaurus theme but only added a style in some cases and didn't change anything in others. The Navbar component was ejected with all its children even though it didn't need to be. The Navbar component (and its children) are labeled as not safe for swizzling. This means that updates to docusaurus can break any changes made to these components, so should only be ejected and changed when necessary and requires checking for updates.

docs/site/src/theme/Navbar/index.js: This component was ejected from Docusaurus 2.x. The changes here are just what docusaurus updated the component to. And now that I type this, I realize that I can just delete it so that the default component gets used instead of the custom one. This means only the default component Navbar/Content is changed by just adding references to the two components mentioned previously (as opposed to the 5 or 6 components that were ejected originally).

Styles: Styles were changed where necessary. If a style was updated, then it was also updated to use tailwind syntax. Two styles were used to remove the need for swizzling default docusaurus components, which as mentioned comes at the cost of maintenance). This approach was necessary because docusaurus reuses a lot of general classes for its components or creates dynamic ones that are suffixed by the relative path to the component.

  • This style selects the wrapper around the search bar so that the "get started" button on the home page displays next to the search. Prevents having to eject Navbar/Search component just to add a class.
    [class^="navbarSearchContainer"] {
      @apply min-[997px]:flex min-[997px]:gap-4
    }  
    
  • This style selects the theme switch (light/dark). Prevents having to make an additional update to the Navbar/Content component.
    button[title^="Switch between dark and light mode"] {
      @apply !text-white hover:bg-sui-blue-primary
    }
    

Test plan

How did you test the new or updated feature?


Release notes

Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.

For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.

  • Protocol:
  • Nodes (Validators and Full nodes):
  • Indexer:
  • JSON-RPC:
  • GraphQL:
  • CLI:
  • Rust SDK:

@ronny-mysten ronny-mysten requested review from amnn and wriches July 8, 2024 01:04
Copy link

vercel bot commented Jul 8, 2024

@ronny-mysten is attempting to deploy a commit to the Mysten Labs Team on Vercel.

A member of the Team first needs to authorize it.

Copy link

vercel bot commented Jul 8, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
sui-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 8, 2024 3:59pm

Copy link
Contributor

@amnn amnn left a comment

Choose a reason for hiding this comment

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

I'm not an expert here, so I'll rely on @wriches to review the CSS and TSX changes -- at the moment, it's difficult to follow which changes correspond to the navbar narrow screen improvement, and which changes are unrelated. Ideally, we could split the PR up into multiple -- one for each logical change -- but if that's difficult, I'll settle for more detail on what the rationale is behind some of the other changes.

The end result does look good though! I can confirm that the sidebar is now visible in the narrow version of the site!

One additional thing I noticed is that from the narrow version of the sidebar, if you hit "Back to main menu", you lose access to the sidebar again, maybe this is avoidable, but it's a bit of a papercut.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a bit difficult for me to follow what these changes are intended to do -- could you add some PR comments to explain?

Some of this stuff I would expect to be done a different way, ideally (like by adding extra class names, rather than trying to select the elements using CSS class hierarchies, etc), but I'm not an expert here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason for that is to minimally change the docusaurus components. This makes upgrades a lot easier. The menu problem is a result of updating to Docusaurus 3 where the changes made to the nav broke the behavior, but only slightly. To add extra class names would mean changing more of the default components.

Comment on lines 10 to 11
import ThemeToggle from "@site/src/components/ThemeToggle";
//import NavbarColorModeToggle from "@theme/Navbar/ColorModeToggle";
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar question here -- how come we are able to replace one component for the other here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops. I need to delete that comment. The new components just act as wrappers for those same docusaurus components that show or hide based on location. Reason being is that it's easier to change behavior in this way without changing the default docusaurus component.

@ronny-mysten
Copy link
Contributor Author

ronny-mysten commented Jul 8, 2024

it's difficult to follow which changes correspond to the navbar narrow screen improvement, and which changes are unrelated.

They are all related...beyond some formatting like putting a line between css entries. Some of the css had to be updated to support the menu and were converted to Tailwind syntax while there. Tailwind was used in the beginning so it should have been used throughout for consistency. Contractor wasn't familiar with tailwind, apparently. A future PR will update all styles to tailwind.

One additional thing I noticed is that from the narrow version of the sidebar, if you hit "Back to main menu", you lose access to the sidebar again, maybe this is avoidable, but it's a bit of a papercut.

This is the default behavior. "Back to main menu" just means back to the top nav options. So, the side menu that is shown is the side menu that applies to the current section (guides, concepts, etc.). When you click one of those, you open that section's side menu. I think what you want is to have all the side menus available at once. We can change it but wouldnt be an insignificant lift and comes with maintenance overhead.

Copy link
Contributor

@amnn amnn left a comment

Choose a reason for hiding this comment

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

Thanks for explaining everything in such detail @ronny-mysten ! It looks good to me in that case. I don't know if you want to have a design consult from Jordan or something? Other than that, it looks good to me.

@ronny-mysten ronny-mysten merged commit 67b18a5 into MystenLabs:main Jul 12, 2024
44 of 47 checks passed
@ronny-mysten ronny-mysten deleted the nav-fix branch July 12, 2024 15:58
tx-tomcat pushed a commit to tx-tomcat/sui-network that referenced this pull request Jul 29, 2024
## Description 

Fixes broken nav on small screens. Jira
[DOCS-383](https://mysten.atlassian.net/browse/DOCS-383) for more info.
Also applies tailwind to some styles, which should eventually be used
exclusively.


[docs/site/src/components/GetStartedLink/index.tsx](https://github.com/MystenLabs/sui/pull/18546/files#diff-ed1e3e4e3d31a916f33152c76fae5faaa7fad41c38d7436f441e0d977f33ac72):
Removed the logic that controls the display of the Get Started link on
the home page to its own component. This should make changing the
location the link navigates to easier and to change the logic behind
visible/hidden and other updates. Should make updates easier because
only need to change the component reference of the docusaurus component
instead of the related logic.


[docs/site/src/components/ThemeToggle/index.tsx](https://github.com/MystenLabs/sui/pull/18546/files#diff-3951ee509e2800c54d91ed3424884e59ce6fd2dd8a8d2e71963223728cdad832):
Removed the logic for showing or hiding the default theme switcher from
the default docusaurus component. Moved to its own component to better
support docusaurus updates in the future. Also makes replacing the
default switcher in the future easier.

deleted components from `theme/Navbar`: These components were swizzled
from docusaurus theme but only added a style in some cases and didn't
change anything in others. The Navbar component was ejected with all its
children even though it didn't need to be. The Navbar component (and its
children) are labeled as `not safe` for swizzling. This means that
updates to docusaurus can break any changes made to these components, so
should only be ejected and changed when necessary and requires checking
for updates.


[docs/site/src/theme/Navbar/index.js](https://github.com/MystenLabs/sui/pull/18546/files#diff-a62ab467816eb68ede0469822b5ed6637ff9af29ab3c5cb3831ccf1b321d5db4):
This component was ejected from Docusaurus 2.x. The changes here are
just what docusaurus updated the component to. And now that I type this,
I realize that I can just delete it so that the default component gets
used instead of the custom one. This means only the default component
`Navbar/Content` is changed by just adding references to the two
components mentioned previously (as opposed to the 5 or 6 components
that were ejected originally).

Styles: Styles were changed where necessary. If a style was updated,
then it was also updated to use tailwind syntax. Two styles were used to
remove the need for swizzling default docusaurus components, which as
mentioned comes at the cost of maintenance). This approach was necessary
because docusaurus reuses a lot of general classes for its components or
creates dynamic ones that are suffixed by the relative path to the
component.
- This style selects the wrapper around the search bar so that the "get
started" button on the home page displays next to the search. Prevents
having to eject Navbar/Search component just to add a class.
    ```
    [class^="navbarSearchContainer"] {
      @apply min-[997px]:flex min-[997px]:gap-4
    }  
    ```
- This style selects the theme switch (light/dark). Prevents having to
make an additional update to the Navbar/Content component.
    ```
    button[title^="Switch between dark and light mode"] {
      @apply !text-white hover:bg-sui-blue-primary
    }
    ```

## Test plan 

How did you test the new or updated feature?

---

## Release notes

Check each box that your changes affect. If none of the boxes relate to
your changes, release notes aren't required.

For each box you select, include information after the relevant heading
that describes the impact of your changes that a user might notice and
any actions they must take to implement updates.

- [ ] Protocol: 
- [ ] Nodes (Validators and Full nodes): 
- [ ] Indexer: 
- [ ] JSON-RPC: 
- [ ] GraphQL: 
- [ ] CLI: 
- [ ] Rust SDK:
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants