Conversation
fbaed76 to
6f946c6
Compare
6f946c6 to
0aa4997
Compare
b3cac73 to
a2938fd
Compare
|
I've included an accessible library, called drag-on-drop, which is actually a wrapper of dragula. The main non-jQuery drag'n'drop libraries I found out, weren't accessibles. |
All possible options
input_focus.mp4
|
dbe9329 to
6d2879d
Compare
I'll come back to this modal-related stuff later on. Still pending to implement as component, that feature is pointed here |
The error status is mislocated. The goal is to place it like that: buuuuuuuut, the script who handles that is actually located in this PR. So, I'll fix it there 🤯 |
2ffa360 to
ca26631
Compare
|
@Crashillo Here's the proto with the read-only version of the survey for non-logged users |
59b536e to
438db3c
Compare
I need @entantoencuanto to solve that out. The index number is a property of a collection in rails. In the readonly forms, that collection includes the "blocks" (title & description, optional field) as a regular question. It's tracked in https://github.com/PopulateTools/issues/issues/1635 |
ahukkanen
left a comment
There was a problem hiding this comment.
I've test driven this as well and I just left few general comments and then a couple of comments regarding the usability.
Overall it looks fine, not everything worked perfectly yet but I understand it's supposed to be that way at this point as mentioned in some of the previous comments above.
ahukkanen
left a comment
There was a problem hiding this comment.
Nice progress on this one too, previous comments were already addressed.
I still have a problem regarding the color contrast issues with the free text fields (both short and long). This is essentially the same comment that I left in the previous review but to emphasize my point, I've also added some WCAG resources to apply colors to the inputs that meet the WCAG criteria.
Could you still fix this issue?
Otherwise it seems good but there is one failing spec that still needs to be fixed:
$ cd decidim-meetings
$ bundle exec rspec spec/system/meeting_registrations_spec.rb -e "Meeting registrations when meeting registrations are enabled and has a registration form when the registration form has file question and file is invalid shows errors for invalid file"
...
1) Meeting registrations when meeting registrations are enabled and has a registration form when the registration form has file question and file is invalid shows errors for invalid file
Failure/Error: expect(page).to have_content("Needs to be reattached")
expected to find text "Needs to be reattached" in "..."
...| id: "#{choice_id}_matrix_row", | ||
| disabled: true %> | ||
| <% if answer_option.free_text %> | ||
| <%= text_field_tag "questionnaire[responses][#{answer_idx}][choices][][custom_body]", |
There was a problem hiding this comment.
The free text color contrast still does not seem quite right, although it is slightly better than last time:
When the input is enabled, the border color against the white background is #d0d3d8 which does not meet the minimum color contrast requirement of 3:1
https://webaim.org/resources/contrastchecker/?fcolor=D0D3D8&bcolor=FFFFFF
More information:
https://www.w3.org/WAI/WCAG21/Understanding/non-text-contrast.html
The same requirement of 3:1 also applies between the input background color and the border color which is right now slightly worse than against the background:
https://webaim.org/resources/contrastchecker/?fcolor=D0D3D8&bcolor=FAFBFC
I also think the enabled input fields have to be clearly distinguishable from the disabled state but meeting the WCAG criteria here should help improving this.
There was a problem hiding this comment.
Also on mobile, I would add a min-width to the matrix questions' free text fields:
Right now it does not necessarily identify as a text element where the user can write something. Plus it may be hard to fill those fields as you can only see 1-2 characters at a time.
There was a problem hiding this comment.
[...] meet the WCAG criteria
Since our designer is off a couple of weeks, what about postpone this matter 'til then? I've already written it down in the pending issue. The solution is actually very simple, just editting the tailwind-config.js. In such way, we can move forward on this.
There was a problem hiding this comment.
Yes sure it is fine for me.
Do you want to look into the min-width before merging or add that to the pending issues list too?
The specs needs to be fixed before merging, though.
There was a problem hiding this comment.
Yeah, we're on it. Just pointing the WCAG issue. I presume we can make it along this morning
77a5e3e to
9c59128
Compare
Fixed the numeration of items in the read only view I've also fixed the failing test in meetings by changing the selector used to check terms of service |
|
@entantoencuanto There is a merge conflict in this PR after merging pages. Could you take a look please? |
c505330 to
781e24d
Compare
|
Conflicts resolved after rebasing |
ahukkanen
left a comment
There was a problem hiding this comment.
I think this is quite close already from being merged. I've left few more comments I noticed during the latest review round.
I have also reported few issues in the pending issues list at #9675 that I noticed while testing this. You can address these also or leave them in the pending issues list (won't be blockers merging this).
ahukkanen
left a comment
There was a problem hiding this comment.
Great work on this one!
This is otherwise ready from my perspective but I noticed one more regression issue after making the login boxes fully clickable.
ahukkanen
left a comment
There was a problem hiding this comment.
Great! Everything looks good for me.
This is just a general question and if it is an actual bug we can move it to the pending issues but I noticed that on mobile I cannot see the question numbers
On larger screen sizes they appear under the border of the page although on some mid-sizes they are cut at the very corner of the screen:
Is this by design or is it a bug?
Bug. I switched the position of the number in mobile. 👍 |
Thanks. But I find the position on the right side confusing when we are in the left-to-right reading world. How about adding it just as an inline-block in front of the question? |
* develop: Redesign: surveys & forms (#9504)
* develop: Redesign: surveys & forms (#9504)
* setup for chunked styles * show.erb chunked for easier manipulation * short, long, multiple & single question templates * button controls * matrix tables * renamed js class * bottom decorator * fix logic * [skip ci] renamed js class * input file in forms * add note * styling sorting question * include dragon library * update btn classes & disabled confirmation modal * Fix linter * Update tests This commit removes stylesheets/decidim/surveys/surveys from lists to check after the removal of its registration on surveys assets config * move the number inside the label * use common button style for file-type question * fix i18n complaints * Move REDESIGN_PENDING message * Fix test with expected questionnaire title format * Recover disabled attribute in submit button * Enable confirm modal when redesign is disabled * Fix announcement texts * Update selector in tests * Fix tests with expected questionnaire title format * Don't authorize_participatory_space twice * Use the position in which responses arrive when the response is of type sorting * Pass all required values in sorting form * Remove unused component * Adapt sorting test * Fix tests with expected questionnaire title format * Fix selector in test * Update test * Adapt tests * Hide sorting questionnaire tests which don't fit with redesign * design readonly form * fix i18n lint * remove obsolete assert * fix comments about surveys * new color form border * fix color inputs * replace links for buttons in tests * wrap input/textarea for adjust the form-error message * expand input if they¡re char limited * move login_boxes to an external css component * responsive login boxes * update tests * set min-width in free text fields of matrix forms * Update selector in test * Calculate position of questions in readonly questionnaires using ids of items to be indexed * Use a different template for title_and_description elements of read only questionnaire * Recover original text displayed for title and description * Fix cell test adding required option * Fix test * Add tests to check the answer idx attribute * Remove extra space * Fix selector in tests to avoid ambiguities * set margin-bottom for surveys as others modules do * glitches on forms * refactor login_boxes * add hover effect * switch number position mobile * adapt rtl Co-authored-by: Eduardo Martinez Echevarria <eduardomech@gmail.com>
🎩 What? Why?
Redesign surveys and forms. Use yield clauses in order to chunk the css assets.
Considerations
New NPM library included
📷 Screenshots