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
[TASK-114] Split out AccountFieldsEditor from AccountSettingsRoute #4709
Conversation
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.
Everything looks good to me!
… (to be used also in different PR)
# Conflicts: # jsapp/js/account/accountSettingsRoute.tsx
to allow other pieces of code to correctly update the profile
.k-select__error { | ||
font-size: sizes.$x12; | ||
line-height: 1.6; | ||
font-weight: 400; | ||
font-style: normal; | ||
color: colors.$kobo-red; | ||
margin-top: sizes.$x6; | ||
} | ||
|
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.
Could this message get positioned somehow so that the layout doesn't jump when it appears/disappears? Also, it would be nice if the error state had the same red border as the TextBox
(when unfocused).
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 layout jumping is actually consistent with how the TextBox error is being displayed. I think this will require us having similar errors in all the common components, and then unfiying the functionality.
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.
I added the styles for the select trigger (for all the types: outline, blue, and gray)
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.
I agree that it's consistent with how the TextBox
works, but I wonder if keeping it consistent is worth it in this case - especially since the refactor of the of the KoboSelect
fields makes them look more distinct from the TextBox
es. You could keep the same layout/position and avoid the content jumping by adding a very short animation to a height property/transform of the error message.
Still, I take your point - this shouldn't hold up this task👍I don't need it to give approval.
Note: the CI is consistently failing during the |
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.
Looking good! I have one last small change to request: the select elements on the old account profile could accept tab focus, but ones in this PR can't. It looks like adding tabindex={0}
to bem.KoboDropdown
(or some other appropriate element in the component tree) would fix that.
Checklist
Description
Internal code improvement in preparation for upcoming feture.
Notes
I will be needing
AccountFieldsEditor
for the TOS feature.Changes here:
AccountFieldsEditor
that displays a selected list of user metadata fields and allows editing their values (locally)AccountSettingsRoute
is usingAccountFieldsEditor
nowKoboSelect
kobo-common
version to latest one (needed it for linter rules for enums)Needed for #4711