mysociety / fixmystreet

This is mySociety's popular map-based reporting platform: easy to install in new countries and regions
http://fixmystreet.org/
Other
501 stars 232 forks source link

Form error identification improvements #5000

Closed lucascumsille closed 3 weeks ago

lucascumsille commented 4 weeks ago

Fixes: https://github.com/mysociety/societyworks/issues/4217

For this one I added an extra commit: https://github.com/mysociety/fixmystreet/pull/5000/commits/6c34076c32de98c8018808aaddba2508d17ed9c6 making the search-help component having the same colour as an error, It's a bit confusing that both of them are related, but they have different colours. Also some cobrands are already using the same colour for errors and "search-help" so I thought standardised wouldn't be a bad idea.

Screenshot 2024-06-11 at 11 38 21 Screenshot 2024-06-11 at 11 38 08

This has two commits: One that covers scenarios where an invalid area has been added. the other one to tackle the main issue, when the input has been left empty

[Skip changelog]

codecov[bot] commented 4 weeks ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 75.09%. Comparing base (616c0e5) to head (5843330). Report is 7 commits behind head on master.

:exclamation: Current head 5843330 differs from pull request most recent head d6ce12b

Please upload reports for the commit d6ce12b to get more accurate results.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #5000 +/- ## ========================================== - Coverage 82.60% 75.09% -7.52% ========================================== Files 393 395 +2 Lines 30770 31649 +879 Branches 4881 5169 +288 ========================================== - Hits 25419 23767 -1652 - Misses 3900 6348 +2448 - Partials 1451 1534 +83 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

lucascumsille commented 3 weeks ago

@dracos This PR is not ready, but I left a comment for the third point, which I'm not sure how to tackle:

SW ref 41: Local RSS feeds and email alerts - part 2

thanks =)

dracos commented 3 weeks ago

On 39 - what they're complaining about here is Firefox's behaviour on an HTML5 required field, which is to show a tooltip that doesn't move when you scroll the page. I think this can be considered a bug in Firefox and nothing we need to deal with, I think it very unlikely someone submits a blank form and tries to scroll the page after? So I don't think this is fixed by this PR, because it's still a required field. But I don't think we need to do anything further.

"The last one is the one that I haven't done, because I'm not sure how to tackle it. " - I think this might have been covered by the change I've made to your other PR on the error, which then displays the error if the email is invalid/empty before submission?

errors.html is used because if you don't pick a feed selection, for example, it shows that error. Or if an illegal choice is submitted. But given we pre-select an option, this is very unlikely to happen, and I don't think there's anything we need to do here either.

"Note: Not related with this ticket but when I tried to put something invalid in the "km" it didn't give me any errors, not sure if we want to make it part of this PR." - I really wasn't fussed if people can't work this out, I'm not sure anyone uses it! So no, think can leave that as well.

lucascumsille commented 3 weeks ago

@lizettal SW ref 41: Local RSS feeds and email alerts - part 2 Will be covered by: https://github.com/mysociety/fixmystreet/pull/4952

However: SW ref 39 is a Firefox bug.

I think this can be considered a bug in Firefox and nothing we need to deal with, I think it very unlikely someone submits a blank form and tries to scroll the page after? So I don't think this is fixed by this PR, because it's still a required field. But I don't think we need to do anything further.

lucascumsille commented 3 weeks ago

@dracos thanks for clarifying all the above, about the Firefox bit I agree, let's leave as it is. I'll move SW 41 to this PR: https://github.com/mysociety/fixmystreet/pull/4952