MBS-10272: Convert the header / navbar to Bootstrap #1129
Conversation
mwiencek
force-pushed
the
react-bootstrap-nav
branch
5 times, most recently
from
9f7ba13 to
a7ac567
Compare
|
Uglify -> Terser commit released to production as part of a hotfix for MBS-10427 |
mwiencek
force-pushed
the
react-bootstrap-nav
branch
from
a7ac567 to
acc3345
Compare
|
Fixed the conflicts here and merged in the recent menu changes that were made (IIRC just replacing /edits/all with /edits, and the new /votes link). |
mwiencek
force-pushed
the
react-bootstrap-nav
branch
from
acc3345 to
656b4e8
Compare
How Bootstrap is set up: We import the custom Bootstrap CSS from design-system, and that's it. For the actual components, I chose *not* to use react-bootstrap for a number of reasons: * It does weird things with context, causing different markup to be rendered on the client and server when hydrating. * Its components obscure what the HTML will actually look like, making the official Bootstrap docs hard to use. * It outputs large component trees (in terms of component hierarchy, not markup), which has performance implications for server-rendered pages. * It doesn't skip pointless client-only work (like creating event handlers) on the server, which also degrades performance. A rough performance comparison of a react-bootstrap based navbar vs. a plain React one (both passed to ReactDOMServer.renderToString) showed the react-bootstrap one to be 5X slower. Even though it was a contrived test for a specific component rather than a whole page, we do plan to make heavy use of Bootstrap on our pages, and I don't think making our pages slower on the server is a good idea if we can avoid it. The only issue with avoiding react-bootstrap was a lack of JS for interactive components like menus. Fortunately, basic implementations that are "good enough" for our purposes are easy to write in React: I added root/static/scripts/common/hooks/useDropdown.js for the menus here.
mwiencek
force-pushed
the
react-bootstrap-nav
branch
from
656b4e8 to
b672d8b
Compare
| }; | ||
|
|
||
| export default withCatalystContext(BottomMenu); |
There was a problem hiding this comment.
Why did you decide to pass $c as a prop rather than use withCatalystContext?
There was a problem hiding this comment.
From what I can see, because the Header already uses withCatalystContext, so BottomMenu isn't referenced from anywhere we can't easily pass $c through already.
| <div className="right"> | ||
| <TopMenu /> | ||
| <BottomMenu /> | ||
| <div className="collapse navbar-collapse flex-md-column"> |
There was a problem hiding this comment.
I was expecting the menus to collapse into one of those clickable bootstrap menus when on phone version, but it does seem to just put them one after another in different lines and nothing else:
(FWIW I'd probably expect to collapse all into one menu when on small screens, and leave the search shown on its own)
| @@ -42,7 +41,7 @@ const TYPE_OPTION_GROUPS = [ | |||
| editor: N_l('Editor'), | |||
| }, | |||
| { | |||
| doc: DBDefs.GOOGLE_CUSTOM_SEARCH ? N_l('Documentation') : null, | |||
| doc: N_l('Documentation'), | |||
There was a problem hiding this comment.
Do we just assume we have GOOGLE_CUSTOM_SEARCH or why this change?
| </a> | ||
| <div className="dropdown-divider" /> | ||
| <a className="dropdown-item" href="/doc/FreeDB_Gateway"> | ||
| {l('FreeDB Gateway')} |
There was a problem hiding this comment.
This no longer exists, we should get rid.
| @@ -103,7 +103,7 @@ const Layout = ({$c, ...props}) => ( | |||
| <Head {...props} /> | |||
|
|
|||
| <body> | |||
| <Header {...props} /> | |||
| <Header $c={$c} /> | |||
There was a problem hiding this comment.
Aren't you using withCatalystContext on Header anwyay?
|
Not sure which of these changes are intentional:
|
With these Bootstrap conversions, it's expected in general for the style to look different in some ways, since it's using the Bootstrap theme developed in the design-system repo. However, I don't think these changes you listed were intentionally designed that way: a full style for the Navbar wasn't developed during the GSoC project. So I'm happy to fix some of these to match the current style (but I would prefer to keep it consistent with the design-system theme where I can, too). |
How Bootstrap is set up:
We import the custom Bootstrap CSS from design-system, and that's it.
For the actual components, I chose not to use react-bootstrap for a number of reasons:
A rough performance comparison of a react-bootstrap based navbar vs. a plain React one (both passed to
ReactDOMServer.renderToString) showed the react-bootstrap one to be 5X slower. Even though it was a contrived test for a specific component rather than a whole page, we do plan to make heavy use of Bootstrap on our pages, and I don't think making our pages slower on the server is a good idea if we can avoid it.The only issue with avoiding react-bootstrap was a lack of JS for interactive components like menus. Fortunately, basic implementations that are "good enough" for our purposes are easy to write in React: I added root/static/scripts/common/hooks/useDropdown.js for the menus here.