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.

Sign up for GitHub

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

Jump to bottom

Issue1093 feedback form should have a headline or subject #1130

Merged
merged 5 commits into from
Mar 4, 2021
Merged

Issue1093 feedback form should have a headline or subject #1130

merged 5 commits into from
Mar 4, 2021

Conversation

miguelvaara
Copy link
Contributor

This pull request includes:

  • A new subject field in the feedback form.
  • The subject of the received email is a combination of a service name and a subject text written by a user.
  • WCAG 2.1 AA level improvements (html label tags and aria-labels) in the feedback form.
  • Language support (fi, sv, en) for the new subject field, html label tags and aria-labels in the feedback form.

@miguelvaara miguelvaara added this to the 2.10 milestone Mar 3, 2021
@miguelvaara miguelvaara requested a review from osma March 3, 2021 16:52
@codecov
Copy link

codecov bot commented Mar 3, 2021 •
edited
Loading

Codecov Report

Merging #1130 (f67a739) into master (6a01b70) will increase coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1130   +/-   ##
=========================================
  Coverage     60.30%   60.30%           
- Complexity     1600     1602    +2     
=========================================
  Files            32       32           
  Lines          4436     4439    +3     
=========================================
+ Hits           2675     2677    +2     
- Misses         1761     1762    +1
Impacted Files Coverage Δ Complexity Δ
controller/WebController.php 14.94% <0.00%> (-0.05%) 84.00 <10.00> (ø)
model/SkosmosTurtleParser.php 56.25% <0.00%> (+1.41%) 19.00% <0.00%> (+1.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 6a01b70...f67a739. Read the comment docs.

@miguelvaara miguelvaara marked this pull request as ready for review March 4, 2021 07:25
Some WCAG 2.1 AA level improvements added to the feedback form + chan…
038e4a3 
…ged argument order in the method WebController.php/sendFeecback
osma
osma requested changes Mar 4, 2021
View reviewed changes
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.

Looks very good! I gave a few small comments and requests for possible improvements.

controller/WebController.php Outdated Show resolved Hide resolved
controller/WebController.php Outdated Show resolved Hide resolved
controller/WebController.php Outdated Show resolved Hide resolved
view/feedback.twig Outdated Show resolved Hide resolved
resource/translations/skosmos_sv.po Outdated Show resolved Hide resolved
@sonarcloud
Copy link

sonarcloud bot commented Mar 4, 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

@osma
Copy link
Member

osma commented Mar 4, 2021

Great work, +1 for merging!

@miguelvaara miguelvaara merged commit 1fc53b3 into master Mar 4, 2021
@miguelvaara miguelvaara self-assigned this Mar 4, 2021
@miguelvaara miguelvaara linked an issue Mar 4, 2021 that may be closed by this pull request
Feedback form should have a headline/subject box #1093
Closed
kouralex
kouralex reviewed Mar 4, 2021
View reviewed changes
resource/css/styles.css
@@ -5,6 +5,10 @@ li, td > a {
font-size: 14px;
}

label {
Copy link
Contributor

@kouralex kouralex Mar 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Global setting - this will affect all labels in all places which is not acceptable. See #799 for more information. Should be changed to be as specific as possible.

Requires further changes.

kouralex
kouralex reviewed Mar 4, 2021
View reviewed changes
controller/WebController.php
if ($fromVocab !== null && $fromVocab !== '') {
$message = 'Feedback from vocab: ' . strtoupper($fromVocab) . "<br />" . $message;
}

Copy link
Contributor

@kouralex kouralex Mar 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Some of these lines may have been there for comprehensibility. It is not a good idea to remove them out of no reason. If one does want to do such operations, it is recommended to be done in a separate commit/branch in order to not confuse the PR.

Does not require further changes.

@osma osma mentioned this pull request Apr 7, 2021
Closed
@osma osma added the WCAG label Nov 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kouralex kouralex kouralex left review comments

@osma osma osma approved these changes

Assignees

@miguelvaara miguelvaara

Labels
enhancement WCAG
Projects
None yet
Milestone
2.10
Development

Successfully merging this pull request may close these issues.

Feedback form should have a headline/subject box
3 participants
@miguelvaara @osma @kouralex