-
-
Notifications
You must be signed in to change notification settings - Fork 43
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
Docusaurus upgrade debugging #133
Conversation
ghost
commented
Jan 24, 2021
- docusaurus-plugin-sass"from 0.1.08 to 0.1.11
- config modification
- Implement Navbar/index.js accordingly
1. docusaurus-plugin-sass"from ^0.1.08 to 0.1.11 2. config modification 3. Implement Navbar/index.js accordingly
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.
I reviewed the site and found a bunch of issues that need to be fixed. I will comment on the PR thread.
docusaurus.config.js
Outdated
disableDarkMode: true, | ||
colorMode: { | ||
defaultMode: 'light', | ||
disableSwitch: false, |
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.
disableSwitch: false, | |
disableSwitch: true, |
This should be true, as our theme doesn't work with dark mode.
We should also remove this patch #130 that was put into place to fix an issue until we upgraded past alpha.66 |
@donghwipark let me know once you've completed those fixes and I will review again. |
@phated I fixed the issues you mentioned above. Please confirm. |
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.
@donghwipark great work on this! I have some cleanup that I'm going to push then I'll merge!
src/css/docs.css
Outdated
.navbar .navbar__search .navbar__search-input { | ||
width: 15.5rem; | ||
.DocSearch.DocSearch-Button { | ||
width: 12.5rem; | ||
height: 2.25rem; | ||
font-size: 1rem; | ||
border-radius: 0; | ||
color: #ffffff; | ||
background-color: #9a2829; | ||
font-family: var(--ifm-font-family-base); | ||
border-radius: var(--ifm-global-radius); | ||
} | ||
.DocSearch.DocSearch-Button:hover { | ||
background-color: #9a2829; | ||
color: #ffffff; | ||
height: 2.25rem; | ||
} | ||
.navbar__items .DocSearch-Button .DocSearch-Search-Icon { | ||
width: 1rem; | ||
height: 1rem; | ||
color: #ffffff; | ||
} |
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.
There's a bunch of issues here, but I'm just going to push a fix.
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.
Specifically, you should have been using the css variables and reviewing the different states, hover/active/etc.
src/css/docs.css
Outdated
.DocSearch.DocSearch-Button { | ||
width: 100%; | ||
margin: 0; | ||
height: 2.25rem; | ||
font-size: 1rem; | ||
border-radius: 0; | ||
color: #ffffff; | ||
background-color: #9a2829; | ||
font-family: var(--ifm-font-family-base); | ||
} | ||
span.DocSearch-Button-Placeholder { | ||
display: block; | ||
} |
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.
Same as above. Also, you added code instead of replacing the navbar__search stuff,
src/css/docs.css
Outdated
button.copyButton_node_modules-\@docusaurus-theme-classic-src-theme-CodeBlock- { | ||
color: var(--ifm-color-white); | ||
color: var(--ifm-color-white); | ||
} |
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 was supposed to be removed as I mentioned. Will push the update.
src/theme/Navbar/index.js
Outdated
@@ -14,11 +14,10 @@ import isInternalUrl from '@docusaurus/isInternalUrl'; | |||
import { useLocation } from '@docusaurus/router'; | |||
|
|||
import SearchBar from '@theme/SearchBar'; | |||
import Toggle from '@theme/Toggle'; | |||
// import Toggle from '@theme/Toggle'; |
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.
Why was this code commented out? Just delete it if that's your solution. I'll push the fix.
src/theme/Navbar/index.js
Outdated
// const onToggleChange = useCallback( | ||
// e => (e.target.checked ? setDarkTheme() : setLightTheme()), | ||
// [setLightTheme, setDarkTheme], | ||
// ); |
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.
Same, and you should have cleaned up the other code that used these values. I'll push.
src/theme/Navbar/index.js
Outdated
{/* {!disableDarkMode && ( | ||
<Toggle | ||
className={styles.displayOnlyInLargeViewport} | ||
aria-label="Dark mode toggle" | ||
checked={isDarkTheme} | ||
onChange={onToggleChange} | ||
/> | ||
)} | ||
)} */} |
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.
Same as above. Don't comment code, just delete it. I'll push.