-
Notifications
You must be signed in to change notification settings - Fork 754
Changelog System for Dart Docs #7056
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
base: main
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @antfitch, 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 significantly refactors the changelog system for Dart documentation, moving from disparate pages to a unified, interactive experience. The new system leverages a hybrid static and client-side approach to ensure optimal performance and search engine visibility while providing users with dynamic filtering and search capabilities. This change centralizes content management and introduces a robust, well-documented architecture for future maintainability. Highlights
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
|
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.
Code Review
This pull request introduces a new changelog system for Dart documentation, featuring a hybrid static/dynamic architecture for improved SEO and user experience. The changes include new Dart components for filtering and displaying changelog entries, along with corresponding SCSS for styling and a detailed architecture document. The implementation appears well-structured, leveraging client-side interactivity for filtering while maintaining static rendering for initial load performance. The new system also includes a clear data model and management guide for changelog entries. Overall, the changes are comprehensive and well-executed, enhancing the usability and maintainability of the changelog page.
|
Visit the preview URL for this PR (updated for commit b61f136): https://dart-dev--pr7056-breaking-changes-migration-9su53cep.web.app |
|
/gemini review |
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.
Code Review
This pull request introduces a well-architected changelog system using a hybrid static/dynamic approach, which is a great improvement for both SEO and user experience. The new architecture is thoroughly documented, and the migration of existing content into the new YAML-based system is a significant step forward. My review focuses on improving the maintainability of the new CSS, ensuring the correctness of the generated HTML, and refining some of the Dart implementation details. Overall, this is a solid contribution.
site/lib/src/components/pages/changelog/changelog_filters_sidebar.dart
Outdated
Show resolved
Hide resolved
… Dart code improvements
|
This is ready for review. @parlough, @ericwindmill, and @kevmoo if you need any changes, let me know. |
ericwindmill
left a comment
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 left a few comments, all of which are small and/or just style opinions.
I am not a web expert or Jaspr expert, so @parlough's opinion would be valuable here.
site/lib/main.dart
Outdated
| // pattern: RegExp('LearningResourceIndex', caseSensitive: false), | ||
| // builder: (_, _, _) => LearningResourceIndex(), | ||
| // ), |
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 shouldn't leave commented out code in the codebase
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.
Removing.
| deprecated('deprecated', 'Deprecated'), | ||
| removed('removed', 'Removed'), | ||
| none('none', 'None') | ||
| ; |
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: This semicolon on it's own line is weird? Maybe run dart fmt
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.
The dart formatter is doing this automatically. Here's the message I get back about why this is here:
The standard Dart formatter (dart format) places this semicolon on its own line to clearly distinguish the list of enum cases from the class-like logic that follows. If you didn't have the constructor or fields, the semicolon wouldn't be necessary at all.
And yeah, that's all I've got. So leaving for now, but maybe something to ask the Dart team to reconsider if it seems too weird?
| box-shadow: 0 4px 24px rgba(0, 0, 0, 0.2); | ||
| width: 90%; | ||
| max-width: 600px; | ||
| max-height: 90vh; |
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 think the max-width should have a larger max width, it looks cramped when the desktop window is full screen.
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.
Aye! I last tested on mobile and forgot to test again on desktop. Changed the desktop version to 900px. Hope that's enough.
|
|
||
| dl { | ||
| display: grid; | ||
| grid-template-columns: max-content 1fr; |
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.
Similar to my above comment, I think this should be something like 2fr 3fr. The content looks cramped because the first column takes up most of the space, but has less content. Alternatively, you could explicitly make the content in column 1 be on two lines, so the max-content width isn't so long.
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.
Added a media query to deal with screen width larger than 600px. Added 2fr 3fr to that.
| } | ||
|
|
||
| dl { | ||
| display: grid; |
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.
The legend popup is pretty is hard to read on mobile. Consider using grid-template-area to make the grid shape change based on the width of the screen. For example, the "type of change" could be listed above the paragraph of explainer text on mobile, and in columns (as it is now) on larger screens
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 should be fixed now. Can you take another look?
|
Let's choose a data serialization strategy for Dash sites before committing this. |
parlough
left a comment
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 is exciting @antfitch! Thanks for diving into this.
I haven't had a chance to do a full review, but thought I would leave my in-progress comments. I'll finish up soon.
| } | ||
| } | ||
|
|
||
| // Helper functions for definition list elements if not available in jaspr |
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.
In the latest release (v0.22) of Jaspr (which we use after 97c6cdd), I added supported for these dom components. So once you update with main, you can remove these functions and use the built-in ones :)
| print('Error rendering markdown: $e\n$st'); | ||
| return text('Error rendering markdown'); |
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'd prefer if we don't catch errors here as it might cause issues to be lost. If some Markdown fails to be parsed, we likely should fix it right away rather than potentially not noticing.
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.
Check out my update. Does it work?
tool/dash_site/pubspec.yaml
Outdated
| markdown: ^7.3.0 | ||
| path: ^1.9.1 | ||
| yaml: ^3.1.3 | ||
| yaml_edit: ^2.2.2 |
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.
It seems this dependency isn't used:
| yaml_edit: ^2.2.2 |
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.
deleted
| for (final entry in _liquidVariables.entries) { | ||
| description = description.replaceAll(entry.key, entry.value); | ||
| if (link != null) { | ||
| link = link.replaceAll(entry.key, entry.value); | ||
| } | ||
| } |
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 manual replacement of Liquid variables doesn't feel too great and I worry it'll cause confusion around why some are working while others aren't.
Since it likely doesn't make sense to run the full templating logic over these strings either, perhaps it's best to just avoid using the template variables in the data and just use the expected values.
I don't expect the current variables used here to change much and if ever needed, they are pretty easy to update with find and replace.
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.
Had Antigravity clean this up. How did it do?
| } | ||
| } | ||
| } | ||
| } No newline at end of file |
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 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.
My IDE was deleting these by default on safe. face palm. Should be fixed now.
| if (entry.link != null) | ||
| div(classes: 'read-more', [ | ||
| a(href: entry.link!, target: Target.blank, [ |
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.
| if (entry.link != null) | |
| div(classes: 'read-more', [ | |
| a(href: entry.link!, target: Target.blank, [ | |
| if (entry.link case final entryLink?) | |
| div(classes: 'read-more', [ | |
| a(href: entryLink, target: Target.blank, [ |
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 ended up having Antigravity fix this. I'm actually worried that we may need to update our documentation to make sure we have the most recent information and we're using the more modern pattern that you suggested. Capturing this here so I can find this comment later.
| span(classes: 'material-symbols-outlined', [ | ||
| text('open_in_new'), | ||
| ]), |
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 have a shared component for this.
| span(classes: 'material-symbols-outlined', [ | |
| text('open_in_new'), | |
| ]), | |
| MaterialIcon('open_in_new'), |
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.
Also flagging Antigravity here. We need to circle back and make sure it's suggesting the most up-to-date solutions.
| ]), | ||
| DashMarkdown(content: entry.description), | ||
| if (entry.link != null) | ||
| div(classes: 'read-more', [ |
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.
Consider updating this to use the already existing Button component. If not, consider adding the text-button class to make the styling and interaction handling more consistent.
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.
Updated! Part of this update included adding a trailing icon to the Button class. Look closely.
| if (entry.subArea != null) 'data-subarea': entry.subArea!, | ||
| 'data-tags': entry.tags.map((t) => t.id).join(','), | ||
| // Store description for client-side search. | ||
| // Warning: This might be large. |
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 not sure we should do this. It bloats the page size quite a bit since everything is loaded.
Could the search instead check the already existing content? If we do keep it in the data, we need to escape any potential HTML in the description.
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.
Removed the 'data-description' attribute which should help with page size bloat and updated the search logic in changelog_model.dart. Take a close look.
| ChangelogEntry.fromMap(change as Map<String, Object?>), | ||
| ); | ||
| } | ||
| } |
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.
Currently this will silently fail if there are no changelog entries, rendering an empty index.
Since that likely indicates a mistake or other regression, consider adding a check that if changesData is null or is empty, throws an error.
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.
Double-check my change. Is it what you need?

Description:
This PR refactors the changelog implementation to use a hybrid static/dynamic architecture and migrates the Dart SDK Chagelog, Breaking Changes page, Language Evolution page to this new system.
Preview:
https://dart-dev--pr7056-breaking-changes-migration-9su53cep.web.app/changelog
Stages for this PR before we merge:
This PR has a few stages. This is the workflow I plan to follow:
First batch of commits: code quality review, make sure nothing is missing that was in Breaking Changes, Language Evolution, Dart SDK Changelog.
Second batch of commits: @antfitch to remove Breaking Changes, Language Evolution, and Dart SDK Changelog from the TOC (please note that we will not be removing the CHANGELOG.md from it's repo, but we will no longer directly link to it in our TOC)
Key Changes:
Architecture: Implemented a new hybrid architecture where changelog content is rendered statically (SSG) for SEO and performance, while client-side filtering is handled via a lightweight JavaScript layer.
Components:
Data Model: Introduced ChangelogEntry model that parses data from YAML on the server and re-hydrates from DOM data- attributes on the client.
Documentation: Added site/doc/changelog_architecture.md detailing the new architecture, data flow, and maintenance guide.
Content: Migrated content to src/data/changelog.yml.
What's after this PR:
After this PR is merged, I will move the Dart doc site changes into the new Changelog system.