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

DOP-4533: FE removal of the SidenavBackButton #1067

Merged
merged 6 commits into from
Apr 23, 2024

Conversation

mayaraman19
Copy link
Collaborator

@mayaraman19 mayaraman19 commented Apr 16, 2024

Stories/Links:

DOP-4533

Current Behavior:

cloud docs production
manual production
MTC production

Staging Links:

cloud-docs staging
manual staging
MTC staging
Open API preview page

Notes:

I did not remove the SideNavBackButton from the OpenAPI component, so that should still exist.

README updates

    • This PR introduces changes that should be reflected in the README, and I have made those updates.
    • This PR does not introduce changes that should be reflected in the README

@mayaraman19 mayaraman19 changed the title removing sidenav back button from sidenav DOP-4533: FE removal of the SidenavBackButton Apr 17, 2024
Copy link
Collaborator

@mmeigs mmeigs left a comment

Choose a reason for hiding this comment

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

This is actually an odd one. So, the OpenAPI component in Snooty is actually only used on an odd OpenAPI preview page in docs-landing - not the ones you linked in cloud-docs. It's a head-fake, for sure. That back button is actually rendered in our redoc fork, I believe!! I'm currently asking a channel in Slack if we should change that back button within redoc for the cloud-docs OpenAPI pages.

As for the preview page... it doesn't even show a back button right now. I think something in the conditionals is broken. Could be interesting to look into for you! But in the meantime, I'm in favor of completely deleting SidenavBackButton as a component. Then, in OpenAPI we can replace that with the new Docs Home button. That makes most sense to me.

Copy link
Collaborator

@rayangler rayangler left a comment

Choose a reason for hiding this comment

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

Current change looks good to me (but will hold off on formally approving based on above discussion).

I agree with the idea of deleting the SidenavBackButton component and removing it or updating it from the existing OpenAPI component. If we update the OpenAPI preview page to use the new "Docs Home" button, we should consider making it a minimal reusable component. This page should be testable by building docs-landing and going to /openapi/preview.

I would say this isn't a blocker though and should maybe time box the effort of figuring out the rendering issue of the button on the OpenAPI preview page.

gap: 6px;
align-items: text-bottom;
svg {
height: 17px;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

horrible little way to ensure that the svg and text of home button slightly align, since the svg is a bit bigger than its content so visually it doesn't line up even though the text and icon are bottom-aligned.

Copy link
Collaborator

@seungpark seungpark left a comment

Choose a reason for hiding this comment

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

minor css improvement below. otherwise LGTM 👍

src/components/Sidenav/DocsHomeButton.js Outdated Show resolved Hide resolved
@mayaraman19
Copy link
Collaborator Author

also, I believe the reason the OpenAPI back button is not rendering is because render() which inserts the back button into the DOM became deprecated with React 18. I've been trying to replace it with createRoot() but haven't had too much luck with getting it working. I'll try to do that by today and if this fails I'll open another ticket for this.

Copy link
Collaborator

@seungpark seungpark left a comment

Choose a reason for hiding this comment

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

minor nitpick above but otherwise LGTM. thanks for dropping the other ticket in 🙏

Copy link
Collaborator

@mmeigs mmeigs left a comment

Choose a reason for hiding this comment

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

Thanks!

@mayaraman19 mayaraman19 merged commit 526107e into feature-breadcrumbs Apr 23, 2024
@mayaraman19 mayaraman19 deleted the DOP-4533 branch April 23, 2024 13:54
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.

4 participants