DOCSP-5888: Make active tab on navbar dynamic #94
Conversation
rayangler
force-pushed
the
DOCSP-5888
branch
4 times, most recently
from
9dc4981
to
03b658f
Compare
There was a problem hiding this comment.
Nice work on a tricky problem!
Also, I know we discussed the failing test before and said you could leave it since I will be addressing soon, but I think you should do this instead:
- Comment out the test so that all tests are passing
- Add a TODO comment there that mentions my ticket number (DOCSP-6123) so we know why it's commented out in the interim
src/constants.js
Outdated
| guides: ['guides'], | ||
| }; | ||
|
|
||
| export const URL_BASES = { |
There was a problem hiding this comment.
Calling this URL_SUBDOMAINS would probably clarify what these strings represent.
src/components/Navbar.js
Outdated
|
|
||
| // Uses location to check which link should be active | ||
| checkForLink = location => { | ||
| if (location.hostname === 'docs.mongodb.com' || location.hostname === 'docs-mongodbcom-staging.corp.mongodb.com') { |
There was a problem hiding this comment.
Let's save these URLs as constants outside of the component.
src/components/Navbar.js
Outdated
| if (location.hostname === 'docs.mongodb.com' || location.hostname === 'docs-mongodbcom-staging.corp.mongodb.com') { | ||
| return this.validateActiveLink(location.pathname, '/', URL_SLUGS); | ||
| } | ||
| if (location.hostname.includes('localhost')) { |
There was a problem hiding this comment.
A developer may be running the site from the aliased http://localhost:8000/, but they can also do so from http://0.0.0.0:8000/ or http://127.0.0.1:8000. We want to account for all of these potential development URLs.
src/components/Navbar.js
Outdated
| const slugs = name.split(token); | ||
| let slug; | ||
| if (slugs[1] === undefined || slugs[1] === '') { | ||
| slug = process.env.GATSBY_SITE; |
There was a problem hiding this comment.
What case is this accounting for? I spent a little time looking into it but never encountered this conditional.
src/components/Navbar.js
Outdated
| }; | ||
|
|
||
| // Matches the slug with keys of URL_BASES or URL_SLUGS to return the index it is found | ||
| checkKeys = (slug, keys, urlItems) => { |
There was a problem hiding this comment.
Working with array indexes can get a bit tricky to read and understand down the road. I think this function could be improved by returning the matching slug if found, and null if not. We could simplify the syntax as well:
checkKeys = (slug, urlItems) => {
const urlMapping = Object.entries(urlItems).find(([, value]) => value.includes(slug));
return urlMapping ? urlMapping[0] : null;
};(Unless there was a reason for using indexes from the beginning!)
| }; | ||
|
|
||
| // Static navprops by default | ||
| this.navprops = `{"links": [ |
There was a problem hiding this comment.
Out of curiosity, does everything fail if the object is passed in as an actual object rather than as a string?
There was a problem hiding this comment.
If I remember correctly, docs-tools parses data.navprops as a string and throws an error otherwise.
src/components/Navbar.js
Outdated
| this.navprops = this.modifyActiveLink(); | ||
|
|
||
| return ( | ||
| <div> |
There was a problem hiding this comment.
Forgot to add this comment yesterday—we probably don't need these surrounding <div>s! If you run into a problem removing them, a Fragment would be a better option.
* DOCSP-5888: Make active tab on navbar dynamic * Change navbar function logic * Skip landing page test * Fix navbar staging bug * Add Navbar test
Staging for guides: https://docs-mongodbcom-staging.corp.mongodb.com/guides/raymundrodriguez/DOCSP-5888/
Staging for spark connector: https://docs-mongodbcom-staging.corp.mongodb.com/spark-connector/raymundrodriguez/DOCSP-5888/
Ticket: https://jira.mongodb.org/browse/DOCSP-5888