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

refactor(frontend): Make dashboard search box the first filter #19721

Merged
merged 2 commits into from
Dec 7, 2022
Merged

refactor(frontend): Make dashboard search box the first filter #19721

merged 2 commits into from
Dec 7, 2022

Conversation

cemremengu
Copy link
Contributor

SUMMARY

Searching is the most natural filter so most people are looking for it first before checking others. This PR moves Search to the beginning of the dashboard filters in an attempt to provide easier accessibility.

Feel free to close this PR if this is not suitable for core team.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

** BEFORE **

image

AFTER

image

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@codecov
Copy link

codecov bot commented Apr 14, 2022 •
edited
Loading

Codecov Report

Merging #19721 (3db08f7) into master (2f23a9c) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #19721      +/-   ##
==========================================
- Coverage   66.85%   66.85%   -0.01%     
==========================================
  Files        1847     1847              
  Lines       70561    70560       -1     
  Branches     7737     7737              
==========================================
- Hits        47174    47173       -1     
  Misses      21380    21380              
  Partials     2007     2007
Flag Coverage Δ
javascript 53.80% <ø> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...rontend/src/views/CRUD/dashboard/DashboardList.tsx 56.60% <ø> (ø)
...omponents/nativeFilters/FilterBar/Header/index.tsx 95.00% <0.00%> (-0.24%) ⬇️
...rontend/src/components/DropdownContainer/index.tsx 72.83% <0.00%> (ø)
.../components/nativeFilters/FilterBar/Horizontal.tsx 100.00% <0.00%> (ø)
...ts/nativeFilters/FilterBar/ActionButtons/index.tsx 78.57% <0.00%> (ø)
...Filters/FilterBar/FilterControls/FilterDivider.tsx 84.00% <0.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@rusackas
Copy link
Member

rusackas commented Dec 5, 2022

@kasiazjc @jess-dillard any objections to this? If not, we can probably get it through quickly. @cemremengu (or anyone, really) would just need to resolve the conflicts

@ghost
Copy link

ghost commented Dec 5, 2022

@rusackas No objections here, let's do it!

@rusackas
Copy link
Member

rusackas commented Dec 5, 2022

@cemremengu let me know if you can fix the merge conflict, and we'll get this thing through!

@cemremengu
frontend(dashboard): Make search the first filter
f552398 
Searching is the most natural filter so most people are looking for it first before checking others. This PR moves `Search` to the beginning of the dashboard filters in an attempt to provide easier accessibility.

Feel free to close this PR if this is not suitable for core team.
@cemremengu
Copy link
Contributor Author

@rusackas done!

rusackas
rusackas requested changes Dec 6, 2022
View reviewed changes
Copy link
Member

@rusackas rusackas left a comment

Choose a reason for hiding this comment

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

One little tweak, then we should be good!

superset-frontend/src/views/CRUD/dashboard/DashboardList.tsx Outdated
@@ -533,13 +539,7 @@ function DashboardList(props: DashboardListProps) {
{ label: t('No'), value: false },
],
},
{
Header: t('Search'),
key: 'search',
Copy link
Member

Choose a reason for hiding this comment

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

Oops! I think this line needed to come along for the ride!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah apologies, forgot to check CI

rusackas
rusackas approved these changes Dec 7, 2022
View reviewed changes
Copy link
Member

@rusackas rusackas left a comment

Choose a reason for hiding this comment

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

LGTM!

@rusackas rusackas merged commit 08f2c9b into apache:master Dec 7, 2022
@cemremengu cemremengu deleted the patch-6 branch December 8, 2022 06:10
This was referenced Jan 3, 2023
Merged
Merged
@sfirke
Copy link
Member

sfirke commented Mar 14, 2023 •
edited
Loading

Could this be done for Charts view also? And I suppose Dataset view as well. Both still have SEARCH last in 2.1.0rc2.

@sfirke sfirke mentioned this pull request Aug 30, 2023
Merged
9 tasks
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 and removed 🚢 2.1.3 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rusackas rusackas rusackas approved these changes

Assignees
No one assigned
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/S 🚢 2.1.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@cemremengu @rusackas @sfirke @mistercrunch