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

GrafanaUI: Add new EmptyState component #84891

Merged
merged 11 commits into from Mar 22, 2024
Merged

Conversation

ashharrison90
Copy link
Contributor

@ashharrison90 ashharrison90 commented Mar 21, 2024

What is this feature?

  • exposes new EmptyState component
    • adds story
    • adds basic documentation
    • this only exposes a search variant for now

image

Why do we need this feature?

  • consistent empty search state for people to use

Who is this feature for?

  • everyone!

Which issue(s) does this PR fix?:

For #84516

Special notes for your reviewer:

see the PoC PR here and associated ephemeral instance for what this looks like when applied in the product

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.

@ashharrison90 ashharrison90 added this to the 11.0.x milestone Mar 21, 2024
@ashharrison90 ashharrison90 self-assigned this Mar 21, 2024
@ashharrison90 ashharrison90 requested review from a team as code owners March 21, 2024 11:38
@ashharrison90 ashharrison90 requested review from leventebalogh, joshhunt and tskarhed and removed request for a team March 21, 2024 11:38
@ashharrison90 ashharrison90 added the area/grafana/ui Issues that belong to components in the @grafana/ui library label Mar 21, 2024

export const EmptySearchState = ({
children,
image = <GrotNotFound width={300} />,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that this should default to not showing the grot image. I think the image should be used sparingly so it's more an occasional surprise/delight, rather than a full-frontal assualt of grots everywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my 2c: i don't think empty search states are a full-frontal assault 😛 they're a very common place for an image. without supporting text (which would be ~80% of the use cases) and without an image this component is very boring 😅

image

};

export const Basic: StoryFn<typeof EmptySearchState> = (args) => {
return <EmptySearchState {...args}>{args.children}</EmptySearchState>;
Copy link
Contributor

Choose a reason for hiding this comment

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

FYIY I don't think you need the {args.children} - spreading args into props should do the trick (<El children={<></>} /> is valid!)

Comment on lines 10 to 24
const DEFAULT_THROTTLE_INTERVAL_MS = 50;

export const useMousePosition = (throttleInterval = DEFAULT_THROTTLE_INTERVAL_MS) => {
const [mousePosition, setMousePosition] = useState<MousePosition>({ x: null, y: null });

useEffect(() => {
const updateMousePosition = throttle((event: MouseEvent) => {
setMousePosition({ x: event.clientX, y: event.clientY });
}, throttleInterval);
window.addEventListener('mousemove', updateMousePosition);

return () => {
window.removeEventListener('mousemove', updateMousePosition);
};
}, [throttleInterval]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you tried instead using requestAnimationFrame as your throttling mechanism? You listen to every mousemove event, but only queue up one setMousePosition call per requestAnimationFrame?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good shout on this 👍 very smooth 🚀

@ashharrison90 ashharrison90 requested a review from a team as a code owner March 21, 2024 16:21
@ashharrison90 ashharrison90 requested review from academo and removed request for a team March 21, 2024 16:21
@ashharrison90
Copy link
Contributor Author

ashharrison90 commented Mar 22, 2024

@joshhunt instead of having separate components (EmptySearchState and EmptyState), have moved towards having a single component with different variants as they're going to end up being basically the same. i think all that will change is the image used.

that will also mean using your preferred pattern of passing button since it's just more flexible and works nicer tbh 😄 see PoC PR for how it will end up looking

@ashharrison90 ashharrison90 changed the title EmptySearchState: Add new empty search state component GrafanaUI: Add new EmptyState component Mar 22, 2024
@breitreiter
Copy link

Do we want an optional CTA that removes any filters applied to the current view? I'm guessing you've arrived at this state by applying a filter set that excludes everything. I imagine the caller would have pass in a function that actually does this work.

@ashharrison90
Copy link
Contributor Author

Do we want an optional CTA that removes any filters applied to the current view? I'm guessing you've arrived at this state by applying a filter set that excludes everything. I imagine the caller would have pass in a function that actually does this work.

yep, you can pass that via the button prop

Copy link
Contributor

@Clarity-89 Clarity-89 left a comment

Choose a reason for hiding this comment

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

Is it me or do both images look the same? 😄

*/
image?: ReactNode;
message?: string;
variant: 'search';
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we document all the props? This will show them in the props table with the description text.

};
}, []);

const styles = useStyles2(getStyles);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Can we move this up with other consts?

@ashharrison90
Copy link
Contributor Author

Is it me or do both images look the same? 😄

naw they have slightly different backgrounds. tbf we could probably get away with just using 1 🤔

@Clarity-89
Copy link
Contributor

Is it me or do both images look the same? 😄

naw they have slightly different backgrounds. tbf we could probably get away with just using 1 🤔

Ah ok, if it's one value inside the SVG maybe we can update it programatically (e.g. set the background based on theme) and use only one SVG?

@ashharrison90
Copy link
Contributor Author

Is it me or do both images look the same? 😄

naw they have slightly different backgrounds. tbf we could probably get away with just using 1 🤔

Ah ok, if it's one value inside the SVG maybe we can update it programatically (e.g. set the background based on theme) and use only one SVG?

i removed the other, imo this one looks fine in either theme 👍

@ashharrison90 ashharrison90 merged commit a0bcc44 into main Mar 22, 2024
18 checks passed
@ashharrison90 ashharrison90 deleted the ash/empty-search-state branch March 22, 2024 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
add to changelog area/frontend area/grafana/ui Issues that belong to components in the @grafana/ui library no-backport Skip backport of PR
Projects
Status: 🚀 Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants