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

Feature: EmptyState component #2175

Merged
merged 12 commits into from
Apr 30, 2024
Merged

Feature: EmptyState component #2175

merged 12 commits into from
Apr 30, 2024

Conversation

hossammourad-okta
Copy link
Contributor

@hossammourad-okta hossammourad-okta commented Mar 18, 2024 •
edited
Loading

https://oktainc.atlassian.net/browse/OKTA-705445

Summary

Create a new generic EmptyState component and its Storybook.

CleanShot 2024-03-19 at 18 03 24@2x

@hossammourad-okta hossammourad-okta changed the title feat: empty state component Feature: EmptyState component Mar 18, 2024
@hossammourad-okta hossammourad-okta marked this pull request as ready for review March 18, 2024 22:22
@hossammourad-okta hossammourad-okta requested a review from a team as a code owner March 18, 2024 22:22
KevinGhadyani-Okta
KevinGhadyani-Okta reviewed Mar 22, 2024
View reviewed changes
Copy link
Contributor

@KevinGhadyani-Okta KevinGhadyani-Okta left a comment

Choose a reason for hiding this comment

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

We're gonna have subtle bugs unless we fix these linting errors:
image

I already made myself a ticket to look into forcing our CI to fail on ESLint warnings.

KevinGhadyani-Okta
KevinGhadyani-Okta reviewed Mar 22, 2024
View reviewed changes
Copy link
Contributor

@KevinGhadyani-Okta KevinGhadyani-Okta left a comment

Choose a reason for hiding this comment

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

Finished my first-pass review.

@KevinGhadyani-Okta
Copy link
Contributor

I believe, last I spoke to @hossammourad-okta, we had a few other changes that were supposed to go in there. For instance, the component should've been named EmptyDataState. I asked him if he could push up his local changes from our last call.

@jordankoschei-okta
Copy link
Contributor

I'm strongly in favor of calling this EmptyState rather than EmptyDataState — this component should be used anywhere we need an empty state, not just in the DataTable and DataStack components.

What other changes do we want to roll into this one? I updated primaryCallToActionComponent and secondaryCallToActionComponent to capitalize the first letters (since they're ReactNodes). Do we have anything else?

KevinGhadyani-Okta
KevinGhadyani-Okta reviewed Apr 29, 2024
View reviewed changes
packages/odyssey-react-mui/src/EmptyState.tsx Outdated
heading: string;
/**
* A descriptive text explaining more context as to why we don't have data.
*/
text: string;
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 this should be named description right? text is a bit generic. The intent is for it to describe the heading similar to an aria-description.

@KevinGhadyani-Okta
Copy link
Contributor

Gretchen confirmed EmptyState.

@KevinGhadyani-Okta KevinGhadyani-Okta merged commit 8c86845 into main Apr 30, 2024
0 of 2 checks passed
@KevinGhadyani-Okta KevinGhadyani-Okta deleted the hm/empty-state-component branch April 30, 2024 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bryancunningham-okta bryancunningham-okta bryancunningham-okta left review comments

@benlister-okta benlister-okta benlister-okta left review comments

@jordankoschei-okta jordankoschei-okta jordankoschei-okta left review comments

@KevinGhadyani-Okta KevinGhadyani-Okta KevinGhadyani-Okta approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@hossammourad-okta @KevinGhadyani-Okta @jordankoschei-okta @benlister-okta @bryancunningham-okta