-
Notifications
You must be signed in to change notification settings - Fork 64
fix(compare): support for Windows newlines and improve formatting #167
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
app/routes/release/compare.tsx
Outdated
| <h4 className="text-lg font-semibold text-[#2f3241] dark:text-white mb-4"> | ||
| <h2 className="text-lg font-semibold text-[#2f3241] dark:text-white mb-4"> | ||
| {groupName} | ||
| </h4> | ||
| </h2> |
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 this change is correct? The "Combined Release Notes" header is h3, so I think h4 here makes sense?
The headers go "Electron Releases" -> "Release Comparison" -> "Combined Release Notes"
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.
Fixed in 9eb23f3 by shifting the rendered Markdown headings from the release notes by +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.
Would adding levelShift: 2 to listMD also fix #155?
| '1': 'text-xl font-bold mb-3', | ||
| '2': 'text-xl font-semibold mb-3', | ||
| '3': 'text-xl font-semibold mb-3', | ||
| '4': 'text-xl font-semibold mb-3', | ||
| '5': 'text-lg font-medium mb-2', | ||
| '6': 'text-base font-medium mb-1', |
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.
Why did we drop the text size classes?
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.
@MarshallOfSound some of the headings in the PR details have text-lg set to them so I wanted to make the heading level appearances more consistent:
release-status/app/routes/pr/details.tsx
Lines 263 to 266 in 566e0fa
| <h3 className="text-lg font-semibold text-[#2f3241] dark:text-white flex items-center gap-2"> | |
| <GitBranch className="w-5 h-5" /> | |
| Backport Information | |
| </h3> |
With the PR in the state it is now, I don't actually think we need this change necessarily (headings generated by markdown-renderer in the compare view are h5 after 9eb23f3), but we could instead do the following to match the non-MD heading styles:
h1: text-xl
h2: text-xl
h3: text-lg
h4: text-lg
h5: text-base
h6: text-base
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 main problem this fixes is that we're using
\r\nfor our major version release notes, so comparisons featuring a major version will be empty.For example:
https://releases.electronjs.org/release/compare/v36.0.0-beta.9/v36.0.0
This PR makes the parsing more resilient by accepting both
\nand\r\n.The rest of the changes kind of follow from there to make the styling a bit better:
##headings are actually<h2>now so headings are semantic.