Skip to content
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

A validation improved (solid red lines around the text fields in all … #1133

Merged

Conversation

miguelvaara
Copy link
Contributor

A validation improved (solid red lines around the text fields in all validated and failed cases). In addition, WCAG 2.1 AA level related global style definition for HTML label tag now applies only to feedback form

…validated and failed cases). In addition, WCAG 2.1 AA level related global style definition for HTML label tag now applies only to feedback form
@miguelvaara miguelvaara requested a review from osma March 9, 2021 17:04
@codecov
Copy link

codecov bot commented Mar 9, 2021

Codecov Report

Merging #1133 (6ae0943) into master (f67a739) will increase coverage by 4.06%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1133      +/-   ##
============================================
+ Coverage     60.30%   64.37%   +4.06%     
- Complexity     1602     1670      +68     
============================================
  Files            32       32              
  Lines          4439     4752     +313     
============================================
+ Hits           2677     3059     +382     
+ Misses         1762     1693      -69     
Impacted Files Coverage Δ Complexity Δ
model/sparql/GenericSparql.php 79.40% <0.00%> (+11.47%) 387.00% <0.00%> (+68.00%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f67a739...6ae0943. Read the comment docs.

Copy link
Member

@osma osma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this locally and it looks like this when I try to submit an empty form:

image

Comments/requests:

  1. The color is now red. I suggest using the pinkish color Skosmos already uses to signal deprecated concepts (see specific comment on CSS file)
  2. The form has the text * Value is required and can not be empty at the bottom, but there are no asterisks next to the Subject and Message fields (or anywhere!). Could you please add asterisks after the labels to mark the required fields?
  3. The default text "Write a subject" in the subject field looks the same as normal text entered into the field. This is a bit confusing, especially since the field will be considered empty for validation purposes even if the default text is there (see screenshot above). Could you please adjust the color/style of the default text so that it doesn't look like regular text, for example by making it a lighter shade?

I noticed that Bootstrap 3 (which Skosmos still uses) has special CSS classes for validation states and we could consider using them instead of a custom CSS class as is done here, but in practice, this would probably be more trouble than it's worth, as we would then have to override much of the Bootstrap styling anyway.

resource/css/styles.css Outdated Show resolved Hide resolved
…border: 2px solid #D95F8A }. Contrast tests for text box with new border style passed

(https://contrastchecker.com/). In the feedback form asterisk symbols added for required fields (subject and message)
Copy link
Member

@osma osma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this locally.

  1. Field outline color is now good!
  2. Asterisks are OK!
  3. The default text still looks too similar to user-entered text:

image

Can you please make the default text a lighter shade, so it doesn't look like the user has actually entered e.g. "Write a subject" into the subject field?

@sonarcloud
Copy link

sonarcloud bot commented Mar 11, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@miguelvaara
Copy link
Contributor Author

Placeholder texts in the input fields are now lighter and italic
Lighter_and_italic

@miguelvaara miguelvaara merged commit b4ce95c into master Mar 11, 2021
@miguelvaara miguelvaara deleted the issue1093-feedback-form-should-have-a-headline_or_subject branch March 11, 2021 14:46
@kouralex kouralex added this to the 2.10 milestone Mar 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants