-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Obs Alerting] Save/use active group state from url !! #197771
base: main
Are you sure you want to change the base?
Conversation
Pinging @elastic/obs-ux-management-team (Team:obs-ux-management) |
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
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.
Thanks Shahzad for picking this up!
When I test locally, the grouping functionality does not work for me:
Screen.Recording.2024-10-25.at.17.32.28.mov
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
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.
...ility/public/components/alert_search_bar/containers/use_alert_search_bar_state_container.tsx
Outdated
Show resolved
Hide resolved
@@ -118,6 +120,21 @@ const AlertsGroupingInternal = <T extends BaseAlertsGroupAggregations>( | |||
onOptionsChange, | |||
}); | |||
|
|||
useEffect(() => { |
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.
How about moving the initialization logic to the grouping context provider?
export const AlertsGroupingContextProvider = ({
initialState = {},
children,
}: PropsWithChildren<{ initialState?: AlertsGroupingState }>) => {
const [groupingState, setGroupingState] = useState<AlertsGroupingState>(initialState);
return (
<AlertsGroupingContext.Provider
value={useMemo(
() => ({ groupingState, setGroupingState }),
[groupingState, setGroupingState]
)}
>
{children}
</AlertsGroupingContext.Provider>
);
};
This way we could avoid the initialization effect.
In case, this is how AlertsGrouping could forward the initialState to the provider:
export const AlertsGrouping = typedMemo(
<T extends BaseAlertsGroupAggregations>(props: AlertsGroupingProps<T>) => {
return (
<AlertsGroupingContextProvider
initialState={
props.initialGroupings
? { [props.groupingId]: { activeGroups: props.initialGroupings } }
: undefined
}
>
<AlertsGroupingInternal {...props} />
</AlertsGroupingContextProvider>
);
}
);
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.
To my understanding this PR tries to sync the URL state with the grouping state. Instead of modifying the grouping component could something like that work?
useEffect(() => {
groupingStateFromURL = getURLGroupingState()
updateGrouping(groupingStateFromURL)
}, [// deps]
…ponents/alert_search_bar/containers/use_alert_search_bar_state_container.tsx Co-authored-by: Maryam Saeidi <[email protected]>
⏳ Build in-progress, with failures
Failed CI StepsHistory
|
🤖 Jobs for this PR can be triggered through checkboxes. 🚧
ℹ️ To trigger the CI, please tick the checkbox below 👇
|
Summary
Fixes #195627
Save/use active group state from url !!
if url state exists it will be propagated to component and mixed with local state