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

Show search button in search bar #4985

Merged
merged 3 commits into from
Apr 21, 2021
Merged

Show search button in search bar #4985

merged 3 commits into from
Apr 21, 2021

Conversation

JammingBen
Copy link
Contributor

Description

Show search button in search bar to make it a11y compliant.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

@JammingBen JammingBen added Status:Needs-Review Needs review from a maintainer Topic:Accessibility labels Apr 20, 2021
@ownclouders
Copy link
Contributor

💥 Visual regression tests webUIFiles2 failed. Please find the screenshots inside ...

https://drone.owncloud.com/owncloud/web/14848/

Diff Image:

topBar.png
Actual Image:

topBar.png
Comparing Against:

topBar.png

💥 Acceptance tests webUIFiles2 failed. Please find the screenshots inside ...

https://drone.owncloud.com/owncloud/web/14848/

20210420-071557-289.png
20210420-071614-960.png
20210420-071955-641.png
20210420-072047-226.png
20210420-072122-280.png
20210420-072154-935.png

pascalwengerter
pascalwengerter requested changes Apr 20, 2021
View reviewed changes
Copy link
Contributor

@pascalwengerter pascalwengerter left a comment

Choose a reason for hiding this comment

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

Did you discuss this further somewhere outside of Confluence? The way I understood it the button could/should only be visually hidden as long as there's no input in the search field and then appear as soon as input.length() > 0 or sth

In any case you may want to delete the example for the Search example with visually hidden button and make sure linter & tests are happy 🤓

@JammingBen
Copy link
Contributor Author

Did you discuss this further somewhere outside of Confluence? The way I understood it the button could/should only be visually hidden as long as there's no input in the search field and then appear as soon as input.length() > 0 or sth

Ahh okay. I only worked with the infos in Confluence, which basically state that the button should be "invisible" for screen readers only. I mean, it's not on me to decide, but buttons which suddenly (dis-)appear based the on input... Sure you want to go this way? :D

@pascalwengerter
Copy link
Contributor

Did you discuss this further somewhere outside of Confluence? The way I understood it the button could/should only be visually hidden as long as there's no input in the search field and then appear as soon as input.length() > 0 or sth

Ahh okay. I only worked with the infos in Confluence, which basically state that the button should be "invisible" for screen readers only. I mean, it's not on me to decide, but buttons which suddenly (dis-)appear based the on input... Sure you want to go this way? :D

Same reservations from my side on this, let me summon @tbsbdr & @kulmann on this and let them decide?

@pascalwengerter
Copy link
Contributor

Did you discuss this further somewhere outside of Confluence? The way I understood it the button could/should only be visually hidden as long as there's no input in the search field and then appear as soon as input.length() > 0 or sth

In any case you may want to delete the example for the Search example with visually hidden button and make sure linter & tests are happy 🤓

Forget about the example stuff, thought we're inside ODS but this is web, after all...

@ownclouders
Copy link
Contributor

💥 Visual regression tests webUIFiles2 failed. Please find the screenshots inside ...

https://drone.owncloud.com/owncloud/web/14867/

Diff Image:

topBar.png
Actual Image:

topBar.png
Comparing Against:

topBar.png

💥 Acceptance tests webUIFiles2 failed. Please find the screenshots inside ...

https://drone.owncloud.com/owncloud/web/14867/

20210420-094457-523.png
20210420-094515-792.png
20210420-094842-737.png
20210420-094917-379.png
20210420-094951-995.png
20210420-095025-247.png

@JammingBen
Copy link
Contributor Author

@individual-it Do you have an idea why the tests fail here?

Visual regression tests webUIFiles2 failed. Please find the screenshots inside ...

I updated the screenshot, but the test still fails.

Acceptance tests webUIFiles2 failed. Please find the screenshots inside ...

Drone complains about failing tests related to sorting, which is not part of this PR at all.

@individual-it individual-it mentioned this pull request Apr 21, 2021
Closed
@individual-it
Copy link
Member

@JammingBen let me look into the updated screenshot, the other failures in the github comment are IMO a bug in the script that uploads the screenshot see #4869

@ownclouders
Copy link
Contributor

💥 Acceptance tests webUIFiles2 failed. Please find the screenshots inside ...

https://drone.owncloud.com/owncloud/web/14897/

20210421-061250-962.png
20210421-061327-049.png
20210421-061421-016.png
20210421-061455-736.png
20210421-061728-426.png
20210421-061804-019.png

@individual-it
Copy link
Member

@JammingBen I've updated the screenshot, please rebase on master, that should make more tests green

@JammingBen
Copy link
Contributor Author

Awesome, thanks!

@ownclouders
Copy link
Contributor

💥 Acceptance tests webUIFiles2 failed. Please find the screenshots inside ...

https://drone.owncloud.com/owncloud/web/14903/

20210421-071234-333.png
20210421-071308-649.png
20210421-071342-216.png
20210421-071417-087.png
20210421-071640-555.png

pascalwengerter
pascalwengerter approved these changes Apr 21, 2021
View reviewed changes
Copy link
Contributor

@pascalwengerter pascalwengerter left a comment

Choose a reason for hiding this comment

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

LGTM!

@AlexAndBear AlexAndBear merged commit b8a628d into master Apr 21, 2021
@delete-merged-branch delete-merged-branch bot deleted the show-search-button branch April 21, 2021 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pascalwengerter pascalwengerter pascalwengerter approved these changes

@kulmann kulmann Awaiting requested review from kulmann

@AlexAndBear AlexAndBear Awaiting requested review from AlexAndBear

@fschade fschade Awaiting requested review from fschade

Assignees
No one assigned
Labels
Status:Needs-Review Needs review from a maintainer Topic:Accessibility
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@JammingBen @ownclouders @pascalwengerter @individual-it @AlexAndBear