-
-
Notifications
You must be signed in to change notification settings - Fork 218
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
chore(#9471): ui nav work image replacement - contact and user management #9505
base: master
Are you sure you want to change the base?
Conversation
2156755
to
f7872f7
Compare
Hi @latin-panda! If you can review this, I have some doubts about this change, and you may know a better way to proceed.
|
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 for sending this Lore!
I only covered 50% of the files, it's slow to review because many new lines and checking things around the new code
So submitting the first half and then I'll continue the other later
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.
@lorerod Looking very good! I reviewed 17 of 23 files, tomorrow I'll continue with the other ones. But submitting some of the feedback and questions now
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 is a lot of code (572 lines) and needs some fixes to align it with our standard. Before I dive into this file, I want to make sure... Do we really need all this? 😅
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.
@latin-panda , the only difference from the default config contact-summary-extras.js file is that I added what is necessary to support immunization. Do you know if there is a more straightforward way to add does to the test config?
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.
The same happens to me with the forms.
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.
Same as before.
This is a lot of code (477 lines) and needs some fixes to align it with our standard. Before I dive into this file, I want to make sure... Do we really need all this? 😅
Maybe we can work out only the necessary
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.
@lorerod I finished reviewing, please find below the other part of the feedback
tests/e2e/visual/contacts/contact-user-hierarchy-creation.wdio-spec.js
Outdated
Show resolved
Hide resolved
tests/e2e/visual/contacts/contact-user-hierarchy-creation.wdio-spec.js
Outdated
Show resolved
Hide resolved
tests/e2e/visual/contacts/contact-user-hierarchy-creation.wdio-spec.js
Outdated
Show resolved
Hide resolved
tests/e2e/visual/contacts/contact-user-hierarchy-creation.wdio-spec.js
Outdated
Show resolved
Hide resolved
tests/e2e/visual/contacts/contact-user-hierarchy-creation.wdio-spec.js
Outdated
Show resolved
Hide resolved
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.
It's almost ready! 🤩 Just some small comments below
tests/e2e/visual/contacts/contact-user-hierarchy-creation.wdio-spec.js
Outdated
Show resolved
Hide resolved
tests/e2e/visual/contacts/contact-user-hierarchy-creation.wdio-spec.js
Outdated
Show resolved
Hide resolved
tests/e2e/visual/contacts/contact-user-hierarchy-creation.wdio-spec.js
Outdated
Show resolved
Hide resolved
@lorerod is it ready for review? I'm available if you need :) |
@latin-panda I'm Sorry. Last week, I was busy fixing the cht-sync e2e failure at the |
Co-authored-by: Jennifer Q <[email protected]>
…ings Co-authored-by: Jennifer Q <[email protected]>
Co-authored-by: Jennifer Q <[email protected]>
Co-authored-by: Jennifer Q <[email protected]>
Co-authored-by: Jennifer Q <[email protected]>
Co-authored-by: Jennifer Q <[email protected]>
Co-authored-by: Jennifer Q <[email protected]>
Co-authored-by: Jennifer Q <[email protected]>
Co-authored-by: Jennifer Q <[email protected]>
Co-authored-by: Jennifer Q <[email protected]>
Co-authored-by: Jennifer Q <[email protected]>
Co-authored-by: Jennifer Q <[email protected]>
Co-authored-by: Jennifer Q <[email protected]>
Co-authored-by: Jennifer Q <[email protected]>
Co-authored-by: Jennifer Q <[email protected]>
Co-authored-by: Jennifer Q <[email protected]>
Co-authored-by: Jennifer Q <[email protected]>
Co-authored-by: Jennifer Q <[email protected]>
Co-authored-by: Jennifer Q <[email protected]>
4551730
to
0a62975
Compare
Description
closes #9471
Code review checklist
Compose URLs
If Build CI hasn't passed, these may 404:
License
The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.