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

Disable sort and search when no bookmarks #3253

Merged
merged 4 commits into from
Sep 16, 2024

Conversation

jotaemepereira
Copy link
Collaborator

@jotaemepereira jotaemepereira commented Sep 11, 2024

Task/Issue URL: https://app.asana.com/0/1204006570077678/1208262956542586/f
Tech Design URL:
CC:

Description

It disables the sort and search buttons in the bookmarks manager and panel, when the user has no bookmarks.

Acceptance criteria

AC#1
Given a user with no bookmarks, when the user opens the bookmarks panel then the sort and search button are disabled

AC#2
Given a user with no bookmarks, when the user opens the bookmarks manager then the sort and search button are disabled

AC#3
Given a user with no bookmarks, when the user adds a bookmark then the sort and search features are enabled

Demo

search-sort-disabled.mov

Steps to test this PR:
Verify the listed acceptance criteria are correct.

Definition of Done:


Internal references:

Pull Request Review Checklist
Software Engineering Expectations
Technical Design Template
Pull Request Documentation

@jotaemepereira jotaemepereira requested review from a team and mallexxx and removed request for a team September 11, 2024 14:19
Copy link
Collaborator

@mallexxx mallexxx left a comment

Choose a reason for hiding this comment

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

Looks good overall but I found an issue: the search is still activated by Cmd+F shortcut or by typing a word with empty results; When clearing the search query empty screen is shown instead of the No bookmarks placeholder

@jotaemepereira
Copy link
Collaborator Author

Looks good overall but I found an issue: the search is still activated by Cmd+F shortcut or by typing a word with empty results; When clearing the search query empty screen is shown instead of the No bookmarks placeholder

Great find @mallexxx. I missed that use case. I’ve added a fix for it, let me know what you think.

@mallexxx
Copy link
Collaborator

@jotaemepereira, better, but we need to disable entering the search mode when there‘s no bookmarks: now the bookmarks popover toolbar grows with the search field hidden when typing a letter or hitting Cmd+F; and search activates by itself if a bookmark is added while the popover is shown; I think we shouldn‘t enter the search mode at all while in empty state

@jotaemepereira
Copy link
Collaborator Author

@jotaemepereira, better, but we need to disable entering the search mode when there‘s no bookmarks: now the bookmarks popover toolbar grows with the search field hidden when typing a letter or hitting Cmd+F; and search activates by itself if a bookmark is added while the popover is shown; I think we shouldn‘t enter the search mode at all while in empty state

Thanks. Fixed!

@mallexxx
Copy link
Collaborator

@jotaemepereira, I‘ve experimented a bit with key handlers and there were some inconsistencies like Find In Page activated when no bookmarks in the Bookmarks Popover or Esc key not deactivating search in Bookmarks Manager;
I ended up adding a Key Equivalent handling view in both the Popover and Manager and opened a PR on top of this PR:
#3278

@jotaemepereira jotaemepereira force-pushed the juan/disable-search-sort-when-no-bookmarks branch from de91ed9 to f6fc346 Compare September 16, 2024 13:04
@jotaemepereira jotaemepereira merged commit 2769776 into main Sep 16, 2024
18 checks passed
@jotaemepereira jotaemepereira deleted the juan/disable-search-sort-when-no-bookmarks branch September 16, 2024 13:20
@jotaemepereira jotaemepereira mentioned this pull request Sep 17, 2024
1 task
ayoy pushed a commit that referenced this pull request Sep 19, 2024
Task/Issue URL: https://app.asana.com/0/1204006570077678/1208328455174895/f

Description:
This fixes the bookmark sort UI tests. I introduced some changes where I broke the tests (#3253); luckily, the tests I helped create found the issue!

The main problem is that now we disable sort and search when we do not have bookmarks. UI tests didn’t account for that, so we tried accessing the sort button without bookmarks, which is now impossible.

I fixed the issue and added UI tests to cover the disable states of the sort button when there are no bookmarks.
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.

2 participants