-
-
Notifications
You must be signed in to change notification settings - Fork 5
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
SF-2997 Refresh training books on step change #2763
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2763 +/- ##
==========================================
- Coverage 79.02% 79.02% -0.01%
==========================================
Files 530 530
Lines 30719 30719
Branches 5021 5016 -5
==========================================
- Hits 24276 24275 -1
Misses 5661 5661
- Partials 782 783 +1 ☔ View full report in Codecov by Sentry. |
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.
Is it possible to have a unit test that recreate the scenario by mimicking the clicks? Your fix does stop the bug occurring, but my concern is there is no way to test for regressions.
Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @josephmyers)
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation-steps/draft-generation-steps.component.html
line 8 at r1 (raw file):
labelPosition="bottom" [disableRipple]="true" (selectionChange)="updateTrainingBooks()"
It feels kind of wrong to have two event handler attributes with the same name.
Are you able to update onStepChange()
to call updateTrainingBooks()
instead of having two selectionChange
attributes?
Code quote:
(selectionChange)="onStepChange()"
labelPosition="bottom"
[disableRipple]="true"
(selectionChange)="updateTrainingBooks()"
4d7763f
to
f4287ff
Compare
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.
Thanks. Last week I was thinking it'd be not worth the time to test the clicking specifically, since there's already coverage for the function itself, but today I realized it was actually quite doable.
Reviewable status: 0 of 75 files reviewed, 1 unresolved discussion (waiting on @pmachapman)
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation-steps/draft-generation-steps.component.html
line 8 at r1 (raw file):
Previously, pmachapman (Peter Chapman) wrote…
It feels kind of wrong to have two event handler attributes with the same name.
Are you able to update
onStepChange()
to callupdateTrainingBooks()
instead of having twoselectionChange
attributes?
Yeah I saw that today. Ooops!
This involved a few overrides on the stepper to get it to look normal, including enabling header hover when a step is incomplete, disabling font size growth for the selected step, and hiding the odd "edit" state which cluttered up the stepper. Also removed the "initial/steps" tab group on the parent and the extra, persistent Back button.
891eea0
to
f8d89ed
Compare
0bf1b50
to
91068b6
Compare
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.
Reviewed 2 of 75 files at r2, 67 of 67 files at r3, 6 of 6 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @josephmyers)
91068b6
to
f22421f
Compare
3e0da99
to
a1160eb
Compare
This is currently baselined off SF-2973 but will be updated once that PR is in.
This change is