-
Notifications
You must be signed in to change notification settings - Fork 184
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) O3-2261: Add a back button to the location picker page #946
base: main
Are you sure you want to change the base?
(feat) O3-2261: Add a back button to the location picker page #946
Conversation
packages/apps/esm-login-app/src/location-picker/location-picker.component.tsx
Outdated
Show resolved
Hide resolved
packages/apps/esm-login-app/src/location-picker/location-picker.component.tsx
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.
Nicely done, @usamaidrsk. LGTM.
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 @usamaidrsk. Couple of small things, but I think this basically looks good.
packages/apps/esm-login-app/src/location-picker/location-picker.component.tsx
Outdated
Show resolved
Hide resolved
packages/apps/esm-login-app/src/location-picker/location-picker.component.tsx
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.
Functionally this looks good in that it provides what is missing. Is this really the UI guideline that carbon advocates for this? I have the same question about the existing "Back" link on the password page that takes you back to the login page, with this odd link at the top of the url for this.
I would have thought things would look at lot more consistent to do this via a button alongside the main save button - eg. a Cancel button that would serve to take you back to the previous step.
For example, we have this pattern on the registration form:
We have this pattern on the cancel dialog:
Aside from these clear inconsistencies, it does seem like a styled button is a better UI than an unstyled link thrown at the top of the form.
Anyone else feel this way?
I see what @mseaton means here. I think the However, in the Location picker here, when opened outside of the login flow, the Back button is less intuitive. We're just leaving the home page to open a new 'full size' page. Therefore, a 'Discard/Confirm' button might make more sense. @usamaidrsk , would you be able to go through both the OpenMRS style guide and designs to find examples or guidelines, as well as Carbon official documentation? |
@rbuisson I do get it now. |
packages/apps/esm-login-app/src/location-picker/location-picker.component.tsx
Outdated
Show resolved
Hide resolved
Hi @mseaton @rbuisson, |
@vasharma05 @usamaidrsk , from my message on Slack, I still suggest:
So let's just change to a standard button and merge that, while we can work on the new location picker later. |
@rbuisson @vasharma05 @denniskigen I have updated this PR as per the suggestions. |
@usamaidrsk , you did put Confirm and Cancel one under the other, as opposed to one next the other. Carbon suggests the latter for modals (I know it's not exactly a modal here, but it looks like one) |
30f2ae2
to
1203e5b
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.
A small nit, but all good to go.
Thanks @usamaidrsk!
packages/apps/esm-login-app/src/location-picker/location-picker.component.tsx
Outdated
Show resolved
Hide resolved
Hi @ibacher! |
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.
Couple of small things, but generally looks good.
Requirements
feat
,fix
, orchore
, among others). See existing PR titles for inspiration.For changes to apps
If applicable
Summary
This PR adds a back button on change location screen, for a user to go back to previous page if they want to cancel the operation
Screenshots
cancel-btn-change-location.mp4
Related Issue
O3-2261
Other