Conversation
…ocation in homepage, because it is now used in multiple areas).
…(in particular, implementing functionality that allows us to display a version used by the coming custom home page).
… that is no longer used, and fulfilling a TODO).
…der.json. If we are in a custom DC, we will look in a particular directory (config/custom_dc) and, if we find a `header.json` in the enclosed custom directory, we will use that. With this, we will be able to use the standard Data Commons header, while along custom DCs to customize the actual header content.
…conBox component that allows us to display descriptions as used on the new custom home page view.
… app, which will display the search bar and the tool list.
…t of items than the primary site's list).
…ws it to be displayed anywhere on the site as a regular content component item.
…and display both the main site's header (with the custom content) as well as the new custom home page content (styling still coming).
… than HEADER_MENU_V2 (which has now been removed). We also explicitly add the tool list because we have now added a stat var explorer tool to the list which isn't to display on the home page (by default we display all the tool types if none are given).
Clean up HTML and Classes from the main wrappers and components, keeping the same logic and functionality
…on the home page.
…ools has a grey intro paragraph.
…ng out the icons into its own map).
• Show Load more now its handled in the beginning • The results can be infinite the wrapper nows scrolls past the visible threshold
…rts into a single line.
With some of the class names having changed, the tests are now updated to compensate. Additionally, the test that counts the number of search results now only counts the actual search results (which number 4 because, in order to properly space the elements in the search results, we lowered the number to display from 5 to 4 to fit the show more equally). This means that we are checking for 4 search results, instead of the previous 6.
…size for places, brings back the cursor:text
This reverts commit 34d5d48.
Summary of ChangesHello @beets, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on a significant modernization of the frontend architecture, particularly for custom Data Commons deployments. It streamlines the configuration of UI elements like headers and footers, introduces a more modular component-based approach for the homepage and search functionality, and updates underlying dependencies. These changes aim to enhance customization capabilities, improve code maintainability, and prepare the application for future feature development. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-executed refactoring of the frontend, migrating the header, footer, homepage, and search components to a more modern, reusable, and theme-based architecture using Emotion. This is a great step forward for maintainability. The backend changes support custom configurations, and tests have been updated accordingly. My review focuses on a key performance improvement opportunity in the backend, along with a minor code cleanup suggestion to further enhance code quality.
server/__init__.py
Outdated
| custom_config_path = get_custom_config_path(custom_dc_template_folder) | ||
| if custom_config_path: | ||
| custom_header_override = os.path.join(custom_config_path, "base", | ||
| "header.json") | ||
| if os.path.exists(custom_header_override): | ||
| header_json_path = custom_header_override | ||
|
|
||
| custom_footer_override = os.path.join(custom_config_path, "base", | ||
| "footer.json") | ||
| if os.path.exists(custom_footer_override): | ||
| footer_json_path = custom_footer_override |
There was a problem hiding this comment.
The logic to determine custom header and footer paths, including os.path.exists checks, is inside the inject_common_parameters context processor. This means it runs on every request, causing unnecessary file system access and impacting performance. This logic should be moved to the create_app function to be executed only once at application startup. The resulting paths can then be stored in app.config and accessed from there in the context processor.
There was a problem hiding this comment.
Add a todo and we can fix this in a follow-up
server/__init__.py
Outdated
| """ | ||
| Return the custom configuration folder path | ||
| if custom configuration is enabled and a custom template folder is provided. | ||
| """ |
There was a problem hiding this comment.
The docstring for get_custom_config_path is not correctly indented according to PEP 257. The content of a multiline docstring should be indented to the same level as the opening quotes. I've also taken the liberty to improve the wording for clarity and adopt a more standard format.
"""Gets the path for a custom configuration folder.
Returns:
The path to the custom configuration folder if custom configuration is
enabled and a folder is provided, otherwise None.
"""
There was a problem hiding this comment.
Thanks @beets !
I successfully tested this with:
DC_API_KEY=<my-api-key> ./run_server.sh -m -e custom
And verified other custom environments work like:
DC_API_KEY=<my-api-key> ./run_server.sh -m -e stanford
lgtm overall, can you just:
- Fix indentation warning raised by gemini
- Merge in the latest master branch
- Reset the mixer and import submodules to match the version on master
server/__init__.py
Outdated
| """ | ||
| Return the custom configuration folder path | ||
| if custom configuration is enabled and a custom template folder is provided. | ||
| """ |
server/__init__.py
Outdated
| custom_config_path = get_custom_config_path(custom_dc_template_folder) | ||
| if custom_config_path: | ||
| custom_header_override = os.path.join(custom_config_path, "base", | ||
| "header.json") | ||
| if os.path.exists(custom_header_override): | ||
| header_json_path = custom_header_override | ||
|
|
||
| custom_footer_override = os.path.join(custom_config_path, "base", | ||
| "footer.json") | ||
| if os.path.exists(custom_footer_override): | ||
| footer_json_path = custom_footer_override |
There was a problem hiding this comment.
Add a todo and we can fix this in a follow-up
|
thanks for the review! i've made the changes, ptal |
|
i also applied the change to avoid looking for config files on every request. tested as well with the commands you shared and things are looking good. ptal |
nick-nlb
left a comment
nick-nlb
left a comment
There was a problem hiding this comment.
Thank you again for doing the rebase and bringing this branch up-to-date!
It's looking and testing well. It just needs a run_test.sh -f to apply formatting changes on the python side before merging in.
We discussed this offline, but just to remind us here, we will wait until after we meet and coordinate with @kmoscoe on before doing the actual merge.
kmoscoe
left a comment
kmoscoe
left a comment
There was a problem hiding this comment.
Could I make a request? Could we remove the other files that are no longer needed in server/templates/custom_dc/custom/ , namely about.html, faq.html and disclaimers.html. Now that we've removed the links to them from the home page, it's just confusing and cluttering to have them hanging around.
|
|
||
| {% block head %} | ||
| <link rel="stylesheet" href={{url_for('static', filename='css/homepage.min.css', t=config['VERSION'])}}> | ||
| <script src={{url_for('static', filename='homepage_custom_dc.js', t=config['VERSION'])}} async></script> |
There was a problem hiding this comment.
Where is this file actually coming from, and what is it used for? I couldn't find anything in static with this filename. Is it supposed to be js/apps/homepage/main_custom_dc.ts? Or does it only get compiled at run time? (I couldn't find it stored locally after starting up the server locally either.)
5 participants
Needed to get it up to HEAD. See that PR for context on changes.