-
Notifications
You must be signed in to change notification settings - Fork 11
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
feat(createScanDialog): issues/1 scan creation options #25
Conversation
Codecov Report
@@ Coverage Diff @@
## master #25 +/- ##
==========================================
+ Coverage 58.94% 61.93% +2.99%
==========================================
Files 83 85 +2
Lines 1929 2002 +73
Branches 316 324 +8
==========================================
+ Hits 1137 1240 +103
+ Misses 665 640 -25
+ Partials 127 122 -5
Continue to review full report at Codecov.
|
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.
as long as the results are as expected, this looks good
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. One small, optional, suggestion.
I see we are losing the ability to maintain expand state when navigating :(
|
||
TouchSpin.defaultProps = { | ||
className: '', | ||
labelMax: <Icon type="fa" name="plus" />, |
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.
might consider an sr-only span to help with accessibility
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: so it looks like we were already hiding the buttons for min and max and just exposing the input field with a aria-hidden
. The field can still be wrapped with a label, so thinking we're safe, least for pf3 accessibility.
edit: admittedly this does bypass the accessibility problem all together, which isn't great
What was broken for sure was the ability to be notified when the field validation updated inside the dialog. So went back and added the aria-live
attributes to the two most recent parts we updated... the Add Source Wizard, and the Scans dialog covered by this PR
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.
... and yeahhhhhh its a little sad on the expanded bits, on the upside removing the subsequent ajax calls the expanded state was forced to handle did improve overall load time
* build, react framework updates * styles, forms clean up, touchspin form adjustments * apiConstants, organize, submit scanName, scanSources added * scanActions, duplicate action addStartScan, associated constant * scansEditReducer, link scanAction, addStartScan, hide show modal type * sources view, move lifecycle concepts to sourcesReducer state * sourcesListItem, restructure and testing updates * redux actions, constants, reducers clean up and testing updates * touchspin, component added * createScanDialog, added scan creation options
* scansEditReducer, submit error messaging * helpers, getMessages expanded
* build, react framework updates * styles, forms clean up, touchspin form adjustments * helpers, getMessages expanded * apiConstants, organize, submit scanName, scanSources added * scanActions, duplicate action addStartScan, associated constant * scansEditReducer, scanAction, addStartScan, display modal, errors * sources view, move lifecycle concepts to sourcesReducer state * sourcesListItem, restructure and testing updates * redux actions, constants, reducers clean up and testing updates * touchspin, component added * createScanDialog, added scan creation options
* build, react framework updates * styles, forms clean up, touchspin form adjustments * helpers, getMessages expanded * apiConstants, organize, submit scanName, scanSources added * scanActions, duplicate action addStartScan, associated constant * scansEditReducer, scanAction, addStartScan, display modal, errors * sources view, move lifecycle concepts to sourcesReducer state * sourcesListItem, restructure and testing updates * redux actions, constants, reducers clean up and testing updates * touchspin, component added * createScanDialog, added scan creation options
What's included
How to test
Coverage and basic unit test check
$ yarn
$ yarn test
Interactive unit test check
$ yarn
$ yarn test:dev
Code
$ yarn
$ yarn start:stage
to start up a local running instance of the API against development codeExample
Updates issue/story
#1 #18