-
Notifications
You must be signed in to change notification settings - Fork 12.4k
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
Nested folders: Add folder actions to other tabs #68673
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good with code, and gave it a spin and works well with nested folders enabled. I want to test as well when disabling the feature flag before giving it my ✔️
export function BrowseFolderAlertingPage({ match }: OwnProps) { | ||
const { uid: folderUID } = match.params; | ||
const { data: folderDTO, isLoading } = useGetFolderQuery(folderUID); | ||
const folder = useSelector((state) => state.folder); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can I be a pain and ask if we can write tests for this?
Something like
describe("BrowseFolderAlertingPage", () => {
it("renders correct breadcrumbs");
it("renders folder name");
it("renders alerts for folder");
it("shows the alerting tab as selected")
});
Likewise for the other one? 🥺
const mockFolderUid = '12345'; | ||
const mockAlertRuleName = 'myAlertRule'; | ||
|
||
const mockRulerRulesResponse: RulerRulesConfigDTO = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to discuss this with the wider frontend group - is there a better home where these fixtures can live? Maybe we could put a TODO on this to find a better home for it?
Personally, I would like to extend this fixtures pattern we/I started in browse-dashboards to the rest of the codebase...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have moved these into separate fixtures files, still in the browse-dashboards
folder for now. should make it very easy to move them later (e.g. if we want to move the libraryElements fixtures to the libraryPanels folder)
What is this feature?
Alerting
andPanels
tabsDashboards
tab)Dashboards
tab)Why do we need this feature?
Who is this feature for?
Which issue(s) does this PR fix?:
Fixes #
Special notes for your reviewer:
Please check that: