-
Notifications
You must be signed in to change notification settings - Fork 2k
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
UI: Add job version revert buttons #10336
Conversation
This is a start, much to be determined/done.
Ember Asset Size actionAs of 4c9ec9c Files that got Bigger 🚨:
Files that stayed the same size 🤷:
|
Ember Test Audit comparison
|
😖 re the ever-growing styling interface
Placing this in boxed-section: questionable? Should there be job-version.scss? 🧐
This happens twice in watchable-namespace-ids, maybe not quite widely-used enough to extract? 🤔
Without this, if the job’s current version is updated outside the UI, the “Revert” button won’t be added to the formerly- current version.
Re-drafted to add post-reversion redirection to overview, as suggested by @DingoEatingFuzz, which will also likely unintentionally fix the known bug 😆 |
As suggested by @DingoEatingFuzz.
Okay, redirection added, now truly ready for 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.
This looks good!
The only thing I'd like verification on (and maybe a test) is if the revert request is correct when not in the default namespace.
JobID: jobName, | ||
JobVersion: jobVersion.number, | ||
}, | ||
}); |
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 forget how this URL building stuff works. Is this going to append the namespace as a query param?
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.
ya, I did manually test that but it makes sense to automate, how does c30917f look for that?
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 good!
2c702dc
to
c30917f
Compare
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
This adds a Revert two-step button to the
JobVersions
component for not-current versions, which redirects to the overview on success:It checks the job version before and after reversion to mitigate the edge case where reverting to an otherwise-identical version has no effect, as discussed in #10337. Wording tweaks welcome!
It uses existing facilities for handling other errors and disabling the button when permissions are lacking.
Implementation notes:
.is-fixed-width
toboxed-section
, would it be better in a newjob-version.scss
? Or elsewhere? 🤔 longing for Tailwind-esque facilities here.has-fading-background
for TSB is very specific to this context, with a white background that fades out to the left 🧐