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

Update skeleton.mdx #3726

Closed
wants to merge 3 commits into from
Closed

Update skeleton.mdx #3726

wants to merge 3 commits into from

Conversation

julauxen
Copy link
Collaborator

@julauxen julauxen commented Feb 15, 2024

Deploy Preview

What does this PR do?

Update skeleton guidance.

Where should the reviewer start?

At guidance.

What testing has been done on this PR?

In addition to the feature you are implementing, have you checked the following:

General UX Checks

  • Small, medium, and large screen sizes
  • Cross-browsers (FireFox, Chrome, and Safari)
  • Light & dark modes
  • All hyperlinks route properly

Accessibility Checks

  • Keyboard interactions
  • Screen reader experience
  • Run WAVE accessibility plugin (Chrome)

Code Quality Checks

  • Console is free of warnings and errors
  • Passes E2E commit checks
  • Visual snapshots are reasonable

How should this be manually tested?

Any background context you want to provide?

What are the relevant issues?

Screenshots (if appropriate)

Should this PR be mentioned in Design System updates?

Is this change backwards compatible or is it a breaking change?

@@ -15,16 +15,19 @@ import { DashBoardSkeletonExample, SkeletonAnatomy } from '../../examples';
<DashBoardSkeletonExample />
</Example>

### Guidance
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wondering if we need this section, feels like it is captured by the introduction already:

Screen Shot 2024-02-15 at 3 48 03 PM

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The info is a bit repeated because I was trying to keep the same structure from the Spinner guidelines, with guidance, went to use, when not to use... We can also add more information to the guidance section like the notifications thing you mentioned in the other comment. What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just for some context on how the pages have been built over time, we haven't quite ironed out the sections that we always want across pages. That being said, Spinner is a bit of an older page so I wouldn't rely on it as the "gold standard" for page structure. Some newer pages are Layer, Card, etc.

I think we realized somewhere along the way that the entire docs site is "guidelines"/"guidance" so having a section called that wasn't super meaningful. I'd still opt for removing this section and just altering the description at the top of the page if we need wording changes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think as a team we should probably revisit what our "gold standard" is for page structure

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, agreed, Oliver and I are starting to look at that.

- The Spinner component may be more appropriate.
- The loading process takes more than a few minutes to load.
- Pair with a message or notification to provide the user with better visibility of a system’s status.
- The data or elements being loaded are dynamic. Use a [Spinner] (/components/spinner) instead.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe skeleton would also apply when the data is dynamic, so I'd remove this section. I'd think we would always know what ultimately would render in a section of a page, because if the data was there, we'd render it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a discussion I was having with @KennyAtHPE... He mentioned that the main difference between skeleton and spinner is that skeleton is only for known content and spinner is for unknown. We changed the word to dynamic because unknow was hard to understand. Do you have a better way to put it? or are you saying that this distinction should not be made?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm more suggesting the distinction should not be made. Since skeleton is aiming to give a scaffold of the page as data is being fetched, I don't think there's really a scenario when elements are unknown. Happy to discuss this further next week on a call if helpful!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will schedule some time for us

- The data or elements being loaded are dynamic. Use a [Spinner] (/components/spinner) instead.
- The user is awaiting feedback for in-context actions such as fetching search results. Use a [Spinner] (/components/spinner) instead.
- The user is awaiting feedback on a submission process, like submitting a form. Use a [Busy Button] (/components/button#busy-button) instead.
- The loading process takes more than a few seconds to load. In that case, pair it with a notification to provide the user with better visibility of a system’s status.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm questioning why this is under "When not to use" -- it feels like we're really saying "Use skeleton, and also use a notification". Can we pull this bullet to elsewhere on the guidance?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed, we can put the in guidance part.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Given my comment above about removing the "guidance" section, how would you feel about adding this in the "When to use" instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not sure, because this is more of a generic guideline... I don't think it fits right in the "when not to use" or in the "when to use" section. That's why I was a unsure about removing the general guidance section, even though I understand that it is redundant. If we do remove it, where do we put the more generic info?

@taysea
Copy link
Collaborator

taysea commented Apr 2, 2024

Closing since skeleton guidance updates were published by #3742

@taysea taysea closed this Apr 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants