-
Notifications
You must be signed in to change notification settings - Fork 37
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
Add collapsible element to updater notification to show changelog, open if there breaking changes #4051
Add collapsible element to updater notification to show changelog, open if there breaking changes #4051
Conversation
QA Wolf here! As you write new code it's important that your test coverage is keeping up. |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@franknoirot I'm trying to push a dummy updater-test build with https://github.com/KittyCAD/modeling-app/tree/updater-test that would include the release notes so we can test this branch with that. They aren't currently being populated on release |
Need to fix this windows failure https://github.com/KittyCAD/modeling-app/actions/runs/11214762223/job/31170760758 before we can move on though New attempt is at https://github.com/KittyCAD/modeling-app/actions/runs/11214995573 |
Great, thank you. I'll still make the component resilient to if release notes aren't present, but that will be very helpful for confirming it's appearance. |
Made good progress, had to rework the way notes were published I had understood the docs wrong. This is what it looks like in an updater test related to #4123
|
Thanks @franknoirot for merging #3995! Just updated from main, hope I didn't mess up conflict resolution |
CUT_RELEASE_PR: true | ||
BUILD_RELEASE: true |
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.
CUT_RELEASE_PR: true | |
BUILD_RELEASE: true | |
CUT_RELEASE_PR: ${{ github.event_name == 'pull_request' && (contains(github.event.pull_request.title, 'Cut release v')) }} | |
BUILD_RELEASE: ${{ github.event_name == 'release' || github.event_name == 'schedule' || github.event_name == 'pull_request' && (contains(github.event.pull_request.title, 'Cut release v')) }} |
Don't let me merge this without reverting this part
🔥 |
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.
LGTM!
…en if there breaking changes (#4051) * Add collapsible element to updater toast notification showing release notes * Temp create release artifacts to test updater * Fix tsc error * Fix some styling, make release notes not appear if no notes are present * Add component tests * Remove test release builds --------- Co-authored-by: Pierre Jacquier <[email protected]>
Closes #3984. Need to test the updater to ensure that we still get the markdown of the release notes to be able to parse them. I think I still need to add error handling if the notes are undefined.
Yeah at least in the updater test I'm not receiving any release notes to parse, so I'm getting an error. Need to test the component out with some fake notes data (oh and I can write a component test for it!), then coordinate with @pierremtb to make sure the app has the release notes markdown when returned from the auto-updater.