-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
[#55801] Add form validation in add project dialog for project attributes #15921
[#55801] Add form validation in add project dialog for project attributes #15921
Conversation
fc1ae41
to
aafd431
Compare
1936e23
to
50292c4
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.
Looks good to me, Judith! 👍🏾 I left some remarks tagged by the traffic light system (🟢 🟠 🔴 )
Some general Qs:
-
🟠 I noticed that we have a bit of a flicker now due to the split dialog render- I presume this is acceptable UX as the
respond_with_dialog
already existed?flicker-add-projects-attributes.mp4
-
🟢 I wonder if there's any further padding or restructuring we can do to the modal body to stop the "X" icon hover state touching the autocompleter input. Right now it's a bit of an eye sore 😬
...oject_custom_fields/project_custom_field_mapping/new_project_mapping_form_component.html.erb
Show resolved
Hide resolved
...oject_custom_fields/project_custom_field_mapping/new_project_mapping_form_component.html.erb
Outdated
Show resolved
Hide resolved
I assume this is the case. After you mentioned it, I saw the flickering too (it only happens on the first time after the page is loaded). I used the pattern described in the lookbook "Async dialogs as turbo streams". Actually I'm not sure how I could fix it.
Well... one of my last code clean ups obviously removed the dialog header text. After readding the header title the eye sore is gone again and the icon hover and the input don't snuggle any more: |
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 addressing the feedback, looking good!
...oject_custom_fields/project_custom_field_mapping/new_project_mapping_form_component.html.erb
Show resolved
Hide resolved
|
df03806
to
a873e1d
Compare
Force push / rebase because of changing the base branch - when I started working on this, I thought it was going into the release/14.2 branch, but it should go into dev. |
OP#55801