-
Notifications
You must be signed in to change notification settings - Fork 45
[FTF-426] Post-nav launch fixes #3022
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
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
f21a3ae to
2742567
Compare
dd8112a to
8115a37
Compare
| import { DEFAULT_LANGUAGE } from './constants'; | ||
| import { writeRedirectToConfigFile, getRedirectCount } from './writeRedirectToConfigFile'; | ||
| import { siteMetadata } from '../../gatsby-config'; | ||
| import { GatsbyNode, Reporter } from 'gatsby'; |
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 wasn't used
| contentOrderedList, | ||
| contentMenu: contentMenuObject, | ||
| script, | ||
| layout: { leftSidebar: true, rightSidebar: true, searchBar: true, template: 'base' }, |
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.
searchBar no longer used
|
|
||
| // Stage 9: Prepend title as markdown heading | ||
| const finalContent = `# ${title}\n\n${content}`; | ||
| const finalContent = `# ${title}\n\n${intro ? `${intro}\n\n` : ''}${content}`; |
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.
All other changes here are prettier - this is the key change.
| const handleLogout = useCallback(async () => { | ||
| if (sessionState.logOut.href && sessionState.logOut.token) { | ||
| try { | ||
| await fetch(sessionState.logOut.href, { |
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.
Replicates the hidden form approach of logging out elsewhere
| behavior: 'smooth', | ||
| block: 'center', | ||
| }); | ||
| const element = activeTriggerRef.current; |
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.
Instead of scrollIntoView, which takes the whole page, localise the scrolling to a particular container housing the nav
8115a37 to
33ca7c8
Compare
kennethkalmer
left a comment
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.
Initial feedback, I'm reconfiguring the review app to have inkeep enabled
| Multi-line JSX comment | ||
| with multiple lines | ||
| */} | ||
| {/_ |
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.
[question] unexpected changes from Prettier?
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.
Yep, Prettier butchered it. I've ignored this file in the config for now, and will review from here how it does with other MDX files (didn't want to disable it wholesale for MDX files)
| )} | ||
| tabIndex={-1} | ||
| {...(page.external && { | ||
| target: '_blank', |
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.
[observation] the Spaces > API References > JavaScript link has the icon, but it doesn't open a in a new window 🤔
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.
Sorry, I did the classic "request review but notice an issue and push after it", this should be in place now
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.
aka Fridays
|
@jamiehenson it looks great btw, apologies for the terse comment earlier |
33ca7c8 to
52eca7d
Compare
52eca7d to
4e40cf6
Compare
4e40cf6 to
78afeab
Compare
Unaware of the comment, but thanks! Ready for another round |
kennethkalmer
left a comment
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.
Thing of beauty
| } | ||
|
|
||
| const content = await response.text(); | ||
| await navigator.clipboard.writeText(content); |
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.
Sadly in Safari this doesn't work, I get NotAllowedError: The request is not allowed by the user agent or the platform in the current context, possibly because the user denied permission logged in the console. That said I think that can be a separate fix as it works in Firefox
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.
Apple tax
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.
We have to pay for the peace offering one way or another 🙄
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.
The ticket namesake for this PR is FTF-426, which reintroduces the external link icon to links in the left nav which go off-site.
Additionally, the PR includes the following items as rapid hotfixes to the launch of the new nav:
PageHeaderincluding links to both view and copy the related page markdown content (made possible now via [WEB-4447] Add MDX to Markdown transpilation with content negotiation #3000)introfrontmatter in generated markdown content as a paragraphTo test, go on the review app and: