-
Notifications
You must be signed in to change notification settings - Fork 89
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
New desktop header #4598
New desktop header #4598
Conversation
* Add semicolon to server script * Update .gitignore for IDE * Moved the onload functionality into a separate function and fixed typing. * Conversion of homepage.html to React. The homepage now passes template variables to main.ts, which initializes a React app. All homepage components are now implemented via this React app. Typing files that could potentially be shared later were put into typing files in the shared directory. * The backend is now sending the topics to the homepage as json, removing the need for json treatment on the front-end. * The header and footer of the base template are now React apps. A note that we will have to convert in a subsequent push all existing onLoad calls for existing React Apps to use an event listener to allow multiple apps to be called on one page from different places. There is also room for some refactoring and cleanup. * Converted the extraction of translated label meta-data into a loop rather than having it be explicit. * Conversion of window.onload calls to window.addEventListener. The trigger for this change is the chance that we might get multiple window.onload assignments in different places (with only the last applied assignment being run). This became an issue when the base template was given JavaScript (for the header and footer). It isn't a further issue yet because those are using addEventListener, but by pre-emptively converting all window.onloads, we prevent future clashes. * With the goal both of abstracting the data from the content of the header and footer (the menus) and to facilitate customization, the base components have been reworked. The metadata passed to the front-end via the Jinja templates have been separated out from the main template, and the dictionaries they tie into are no longer strict TypeScript objects, but records. They store as keys the original text, and as a value the translation. The objects themselves are proxies that return the original text back if a translation does not exist. This makes the interaction between the templates, the TypeScript/JavaScript and the React components more flexible. The routing has been reworked in the same way. The header and footer menus themselves are json files located in a data directory inside the base app section. They can be overridden to make custom menus by saving them as "header_custom_dc.json" in the same directory and customizing that file (likewise with footer). The webpack configuration is updated to facilitate these optional imports. * The json that supplies the header and footer menu items is now moved to the backend and injected into the context of every endpoint. We still need to consider a simple way to allow for the overriding of this for custom templates. * Update .gitignore for IDE * Moved the onload functionality into a separate function and fixed typing. * Conversion of homepage.html to React. The homepage now passes template variables to main.ts, which initializes a React app. All homepage components are now implemented via this React app. Typing files that could potentially be shared later were put into typing files in the shared directory. * The backend is now sending the topics to the homepage as json, removing the need for json treatment on the front-end. * The header and footer of the base template are now React apps. A note that we will have to convert in a subsequent push all existing onLoad calls for existing React Apps to use an event listener to allow multiple apps to be called on one page from different places. There is also room for some refactoring and cleanup. * Converted the extraction of translated label meta-data into a loop rather than having it be explicit. * Conversion of window.onload calls to window.addEventListener. The trigger for this change is the chance that we might get multiple window.onload assignments in different places (with only the last applied assignment being run). This became an issue when the base template was given JavaScript (for the header and footer). It isn't a further issue yet because those are using addEventListener, but by pre-emptively converting all window.onloads, we prevent future clashes. * With the goal both of abstracting the data from the content of the header and footer (the menus) and to facilitate customization, the base components have been reworked. The metadata passed to the front-end via the Jinja templates have been separated out from the main template, and the dictionaries they tie into are no longer strict TypeScript objects, but records. They store as keys the original text, and as a value the translation. The objects themselves are proxies that return the original text back if a translation does not exist. This makes the interaction between the templates, the TypeScript/JavaScript and the React components more flexible. The routing has been reworked in the same way. The header and footer menus themselves are json files located in a data directory inside the base app section. They can be overridden to make custom menus by saving them as "header_custom_dc.json" in the same directory and customizing that file (likewise with footer). The webpack configuration is updated to facilitate these optional imports. * The json that supplies the header and footer menu items is now moved to the backend and injected into the context of every endpoint. We still need to consider a simple way to allow for the overriding of this for custom templates. * Updated the home page to use the newer routing flow introduced during work on the base section. * Update Ids set in loop to be set based on a sluggified version of the label. --------- Co-authored-by: Jennifer Blumberg <[email protected]>
- The Github Repository link is added into the footer. - A fix has been applied so that boolean flags pulled from the templates are recognized regardless of the case (this solves the issue where the full footer was always displaying). - TRANSLATORS tags are returned to the templates. Note that some `trans` tags did not have TRANSLATOR tags in the original templates (this is easier to see now that the tags are all in one place). - Comments have been added to functions exported from utilities. - general.ts has been folded into base.ts.
…data-hompeage" (for future collision-proofing), added copywrite note and descriptor to typing files that did not have them.
…e page as the new homepage diverges, we are forking the new homepage to a "_v2". Currently, we are only generating a version two of the JavaScript file, but as we restyle, we will generate a version 2 of the CSS as well. This way, templates that include the old .js and .css files will continue to work as they do now.
… page, and some minor TypeScript fixes. We also added in some missing comments.
We were somewhat limited in how efficiently we could clean up the labels with macros because of the translators tags.
…and non-existent routes now throw errors.
…as not been styled to match the design. As part of this commit, we have defined the header structure for the data source that populates the rich menu (a somewhat more complicated structure than the previous header). We have separated the natural languages search bar component into a parent that calls one of two variant implementations in order to accommodate both the header version (which will be quite different in how it looks) and the standard version.
…uch as feeback) that do not trigger a rich menu. This also pushes an initial (very WIP) setup for the mobile menu.
… single bottom border sliding to accommodate the size of the content. Added some transitions to animate the incoming content.
…te that this is still substantially unstyled.
…lable to all pages. The header CSS is now renamed to core (and will also provide footer CSS).
…(so that the menu does not follow the user down the screen). This is an issue only if the user uses the scroll wheel while the menu is open.
…er/datacommons-website into feature/header-mobile
# Conflicts: # server/config/base/header_v2.json # static/js/apps/base/components/header_bar.tsx # static/js/apps/base/components/menu_desktop.tsx # static/js/apps/base/components/menu_desktop_rich_menu.tsx # static/js/shared/types/base.ts # static/webpack.config.js
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 for this - it's nice to see it working across the site. i added some comments - they are mostly very minor and have to do with naming. could you add them as todo's and we can merge this.
server/config/base/header_v2.json
Outdated
{ | ||
"title": "Other Data Commons", | ||
"items": [ | ||
{ | ||
"title": "Biomedical Data Commons", | ||
"url": "https://docs.datacommons.org/datasets/Biomedical.html", | ||
"type": "external", | ||
"description": "Data Commons has invested into harmonized other domain specific topics like Biomedical data" | ||
} | ||
] | ||
}, |
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.
please drop this section for now.
@@ -444,7 +444,8 @@ def add_language_code(endpoint, values): | |||
def inject_common_parameters(): | |||
common_variables = { | |||
'HEADER_MENU': json.dumps(libutil.get_json("config/base/header.json")), | |||
'FOOTER_MENU': json.dumps(libutil.get_json("config/base/footer.json")) | |||
'FOOTER_MENU': json.dumps(libutil.get_json("config/base/footer.json")), | |||
'HEADER_MENU_V2': json.dumps(libutil.get_json("config/base/header_v2.json")), |
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.
could you add a todo here to replace HEADER_MENU
server/config/base/header_v2.json
Outdated
[ | ||
{ | ||
"label": "Overview", | ||
"ariaLabel": "Data commons overview", |
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.
"ariaLabel": "Data commons overview", | |
"ariaLabel": "Data Commons overview", |
static/css/core.scss
Outdated
|
||
// Styles | ||
|
||
#main-nav-v2 { |
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.
Can we drop the -v2 prefixes? Let's assume we're going forward with the new design..
static/css/core.scss
Outdated
transition: max-height 0.4s ease-in-out; | ||
} | ||
|
||
.rich-menu-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.
super nit: pelase add a space before {
align-items: stretch; | ||
gap: 32px; | ||
margin: auto; | ||
max-width: 1920px; |
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'm hoping these will get lifted up as variables where it makes sense
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.
Yes! We've already started doing that in the next branch coming in.
const handleMouseLeave = (): void => { | ||
resetMenu(); | ||
}; |
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 seems very eager to close the menu. i wonder if we'll need a timer before triggering the menu close - wdyt @miss-o-soup ?
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've added a little timer (that we can easily remove or change the duration of) to make it a bit less eager to close. Perhaps @miss-o-soup we could do a screen-sharing demo and pick a good duration together? I'll add in a TODO to verify the duration.
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.
nit: could we add a header_bar subfolder here, and move the header related files there? since "menu_desktop" sounds like a generic component but it's really header specific. thanks
open, | ||
}: MenuDesktopRichMenuProps): ReactElement => { | ||
return ( | ||
<div className={`rich-menu-content ${open ? "open" : ""}`}> |
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 looks like the only difference between desktop and mobile -- can we fold both into the same component (ok to add a todo for 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.
I've added this TODO in. I won't do the actual folding just yet, in case over the review process the two components diverge significantly (probably unlikely).
… rich desktop menu container. This push also contains a rename of the class, removing the v2 in anticipation of moving over to the new design, a capitalization fix and removal of a section in the content of the headers.
…e closing of the menu.
…eader menu when ready.
…obile components together (if no changes come to fork their functionality).
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.
Looks great!
@@ -0,0 +1,203 @@ | |||
[ |
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'll have to think about a good way to add translations to these strings (in a follow-up pr)
@@ -0,0 +1,74 @@ | |||
/** |
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.
(For a follow-up PR): When scrolling down the page with the menu open, the menu sticks to the top.
### Description This PR is an initial version of the desktop menu for the homepage revamp. The mobile menu is temporarily removed, so that the desktop menu can be reviewed in isolation. *This PR is not intended for merge into master until the completion of the homepage revamp* ### Notes There is still some refactoring and cleanup to do (particularly in the CSS) that will be done as part of a subsequent larger PR that includes the mobile menu. However, the functionality of the desktop can be previewed. --------- Co-authored-by: Nicholas Blumberg <[email protected]> Co-authored-by: Nick B <[email protected]> Co-authored-by: Pablo Noel <[email protected]>
Description
This PR is an initial version of the desktop menu for the homepage revamp. The mobile menu is temporarily removed, so that the desktop menu can be reviewed in isolation.
This PR is not intended for merge into master until the completion of the homepage revamp
Notes
There is still some refactoring and cleanup to do (particularly in the CSS) that will be done as part of a subsequent larger PR that includes the mobile menu. However, the functionality of the desktop can be previewed.