-
-
Notifications
You must be signed in to change notification settings - Fork 220
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
refactor: extract label generation for Edit Task modal #2822
refactor: extract label generation for Edit Task modal #2822
Conversation
Only the snapshot without the access keys is impacted
Only the snapshot without the access keys is impacted
…sskey-first Also tested manually in demo vault - each access key worked
<form> doesnt need the optional class anymore, since this is managed by withAccessKeys variable only
Added jsdocs and refactored a bit =) No more pushes, ready for review =) |
Hi @ilandikov, I started doing a review and then saw you're still pushing. That's absolutely fine - could you just let me know when it's ready for review please? |
Cancel that - I've just seen your comment... |
Thanks... I've given this a once over, but it's too big to review in one evening. Since it exists, I don't think there's any point you doing it all over again, and I would likely have the same comments... I'll review my draft comments on Tuesday, or worst case, Wednesday evening, and get back to you... |
The trajectory of this PR is quite linear, so splitting it into several PR is pretty straightforward. Are you sure you don’t want me to do this?) or you can see it this way - the only functionality I’m touching are the access keys. They work as expected, so the main questions is “is the state of the code better now?” |
OK, that's a helpful comment. In that case, I'll go ahead and submit my comments after a first half hour or so of reviewing just on GitHub... Then you'll have time to address anything you care to, and my next run through the code might be a little easier... |
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.
Thank you for this.
Here are some thoughts after a first read through...
...est.Edit_Modal_HTML_snapshot_tests_should_match_snapshot_-_without_access_keys.approved.html
Show resolved
Hide resolved
Thanks! |
Thanks a lot for your review! Let's discuss the details this WE |
Description
Sorry for a big PR =) I can split it into several ones if needed.
accesskey-first
and unify access key declaration withaccesskey
class<label>
elements O_o<input>
elements which inhibited the access keyseditTaskLabelContent()
function to generate the label content<label>
to<Dependency>
and<DateEditor>
component respectivelyMotivation and Context
<Dependency>
and<DateEditor>
)How has this been tested?
Unit test made possible with thanks to a previous PR with @claremacrae
Manual test at the end of the refactoring:
Types of changes
refactor
- non-breaking change which only improves the design or structure of existing code, and making no changes to its external behaviour)test
- additions and improvements to unit tests and the smoke tests)Checklist
yarn run lint
.Terms