-
Notifications
You must be signed in to change notification settings - Fork 36
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
Conversation
There was a problem hiding this 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.
There was a problem hiding this 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; |
There was a problem hiding this comment.
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.
There was a problem hiding this 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 👍
also, I believe the reason the OpenAPI back button is not rendering is because |
There was a problem hiding this 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 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
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