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

BrowseDashboards: Add RadioButtonGroup to be able to chose between 'Browse' or 'List' view #77561

Merged
merged 9 commits into from
Nov 7, 2023

Conversation

eledobleefe
Copy link
Contributor

What is this feature?
This PR restores the "List view" button.

Why do we need this feature?
To let the user select the most appropriate view for their needs.

Who is this feature for?
Everybody

Which issue(s) does this PR fix?:

Fixes #77121
Special notes for your reviewer:

Please check that:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

@eledobleefe eledobleefe added area/frontend no-backport Skip backport of PR no-changelog Skip including change in changelog/release notes labels Nov 2, 2023
@eledobleefe eledobleefe self-assigned this Nov 2, 2023
@grafana-delivery-bot grafana-delivery-bot bot added this to the 10.3.x milestone Nov 2, 2023
@eledobleefe eledobleefe marked this pull request as ready for review November 3, 2023 16:59
@eledobleefe eledobleefe requested a review from a team as a code owner November 3, 2023 16:59
@eledobleefe eledobleefe requested review from joshhunt and ashharrison90 and removed request for a team November 3, 2023 16:59
Copy link
Contributor

@ashharrison90 ashharrison90 left a comment

Choose a reason for hiding this comment

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

couple of q's, otherwise looks good 👍

@@ -10,7 +10,6 @@ export function BrowseFilters() {
return (
<div>
<ActionRow
hideLayout
Copy link
Contributor

Choose a reason for hiding this comment

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

i know i've asked this before and i can't quite remember the response... are we expecting to be able to remove this when nestedFolders GAs or not? 🤔 if so, we could do something like hideLayout={config.featureToggles.isEnabled('nestedFolders')}. if not, disregard this! 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we are not going to remove it but I might be wrong. @joshhunt can you tell us the answer? We will remember it as it will be written down 😅

Comment on lines 68 to 73
// Set list layout since folders layout implies sort to be undefined
stm.onLayoutChange(SearchLayout.List);

// Verify that they have been set
expect(stm.state.query).toBe('hello');
expect(stm.state.sort).toBe('alpha-asc');
expect(stm.state.sort).toBeUndefined();
Copy link
Contributor

Choose a reason for hiding this comment

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

can you explain these changes a bit more? i don't understand. the comment implies that folder layout will make sort undefined, but then you set to list layout and sort is still undefined. i think if you set list layout before calling initStateFromUrl() this test can remain the same as it was? 🤔

@eledobleefe eledobleefe added add to changelog and removed no-changelog Skip including change in changelog/release notes labels Nov 7, 2023
@eledobleefe eledobleefe changed the title BrowseDashboards: 'List All Dashboards' view BrowseDashboards: Add RadioButtonGroup to be able to chose between 'Browse' or 'List' view Nov 7, 2023
Copy link
Contributor

@ashharrison90 ashharrison90 left a comment

Choose a reason for hiding this comment

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

test looks good now 👍

@eledobleefe eledobleefe merged commit 8f49ec9 into main Nov 7, 2023
22 checks passed
@eledobleefe eledobleefe deleted the eledobleefe/list-all-dashboards-view-77121 branch November 7, 2023 15:16
@aangelisc aangelisc modified the milestones: 10.3.x, 10.2.3 Dec 21, 2023
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.

BrowseDashboards: 'List All Dashboards' view
3 participants