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

[full-ci] Fix search bar display on small screens #7675

Merged
merged 8 commits into from
Oct 5, 2022

Conversation

JammingBen
Copy link
Collaborator

@JammingBen JammingBen commented Sep 22, 2022

Description

We've fixed the display of the search bar on small screens.

We also removed the label of the currently used theme as the icon + tooltip is indicator enough.

Related Issue

Screenshots:

See issue.

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

@JammingBen JammingBen self-assigned this Sep 22, 2022
@ownclouders
Copy link
Contributor

ownclouders commented Sep 22, 2022

Results for oCISFiles2 https://drone.owncloud.com/owncloud/web/28686/57/1

💥 The acceptance tests failed on retry. Please find the screenshots inside ...

webUIFilesSearch-search_feature-L121.png

webUIFilesSearch-search_feature-L121.png

webUIFilesSearch-search_feature-L141.png

webUIFilesSearch-search_feature-L141.png

webUIFilesSearch-search_feature-L78.png

webUIFilesSearch-search_feature-L78.png

webUIFilesSearch-search_feature-L95.png

webUIFilesSearch-search_feature-L95.png

@JammingBen JammingBen changed the title Fix search bar display on small screens [full-ci] Fix search bar display on small screens Sep 22, 2022
Comment on lines -53 to -55
},
switchText() {
return this.isLightTheme ? this.$gettext('Light mode') : this.$gettext('Dark mode')
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unrelated but fixed in the course of this: removed the label of the currently used theme as the icon + tooltip is indicator enough.

@JammingBen
Copy link
Collaborator Author

@individual-it One acceptance test keeps failing here: https://drone.owncloud.com/owncloud/web/28569/57/14. However, on my local machine, the test succeeds. I have no idea what's the issue here... Maybe your team could have a look if time permits?

@amrita-shrestha
Copy link
Contributor

amrita-shrestha commented Sep 27, 2022

@individual-it One acceptance test keeps failing here: https://drone.owncloud.com/owncloud/web/28569/57/14. However, on my local machine, the test succeeds. I have no idea what's the issue here... Maybe your team could have a look if time permits?

@JammingBen, I was working separately to solve acceptance failure in #7692 PR. But mistakenly I pushed my commit to this PR in place of #7692 PR. Sorry for inconvience. You can drop my commit.

From my investigation, for webUIFilesSearch/search.feature:125 scenario
I found that the search doesn't work properly. Test run speed could be the reason .
I tried different ways to solve that problem i.e

  1. added pause for 500ms before search (tests don't pass in 500ms but tests pass in 900ms)
  2. added reload page step before search (flaky)
  3. I tried the combination of 1 and 2 then the test passed.

I found something odd in the search feature. In the tests run, I noticed that after renaming resources or uploading resources Personal page contained renamed/uploaded resources. But if we search currently renamed/uploaded, then renamed/uploaded resources can't be found. This issue is reproducible only in automation testing.

@JammingBen
Copy link
Collaborator Author

@amrita-shrestha

I found something odd in the search feature. In the tests run, I noticed that after renaming resources or uploading resources Personal page contained renamed/uploaded resources. But if we search currently renamed/uploaded, then renamed/uploaded resources can't be found.

That's strange though, as this PR only touches visuals. Also, the failing test suite is for iPhone only (meaning on small screens).

Are you sure that the search bar is being accessed the right way during the test? This PR changed the behavior on mobile devices so the search icon has to be clicked first, then the search input appears. Maybe the time between the click and the appearing search input is too small and we need to wait for the search input to be fully visible and usable?

@amrita-shrestha
Copy link
Contributor

@JammingBen the indexing of uploaded files for search done async could be the culprit behind webUIFilesSearch/search.feature:125 scenario failing. I will look for a way to verify upload has been completed before the search

@individual-it
Copy link
Member

I believe that it worked before because the item '#files-open-search-btn' did not exist in the DOM, so the test would look for it till an timeout was reached and that gave the backend enough time to index the files.
Because @JammingBen put a valid selector for openSearchButton the step runs faster and sometimes there is not enough time to index the file

@JammingBen
Copy link
Collaborator Author

@individual-it I added a pause statement as discussed in the daily yesterday: f77d040

Is that okay from your side (at least for now)?

@individual-it
Copy link
Member

@JammingBen yes lets do it that way. I cannot think of any other way to do it :-(

@JammingBen JammingBen marked this pull request as ready for review September 30, 2022 10:08
},
onResize() {
const searchBarEl = document.getElementById('files-global-search-bar')
if (window.innerWidth > 639) {
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldnt this be possible via css? I mean its fine with me just weird to see this number in code rather than css

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We need it unfortunately to set the visible state back again if you search on a small screen and resize your screen to full again. Without, the search would stay hidden because showSearchBar() actively sets style.visibility = 'visible'.

@sonarcloud
Copy link

sonarcloud bot commented Oct 5, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

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

9.5% 9.5% Coverage
0.0% 0.0% Duplication

@JammingBen JammingBen merged commit 468abaf into master Oct 5, 2022
@delete-merged-branch delete-merged-branch bot deleted the searchbar-on-small-screens branch October 5, 2022 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Search icon is misplaced on small screens
6 participants