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

[EUI+] Add missing guidelines #8309

Conversation

weronikaolejniczak
Copy link
Contributor

@weronikaolejniczak weronikaolejniczak commented Feb 6, 2025

Summary

This PR adds the missing guidelines for several components: EuiEmptyPrompt, EuiToast, EuiTour, Form controls and Selection controls.

QA

Compare the content of all pages between the old docs and the new docs:

Note

There are missing icons in EuiEmptyPrompt preview. It's been extracted to a separate task: #8312 that will be tackled by @tkajtoch.

@weronikaolejniczak weronikaolejniczak self-assigned this Feb 7, 2025
@weronikaolejniczak weronikaolejniczak force-pushed the fix/eui-plus-missing-content branch from acdfd9f to ca0be91 Compare February 7, 2025 13:23
@weronikaolejniczak weronikaolejniczak marked this pull request as ready for review February 7, 2025 13:42
@weronikaolejniczak weronikaolejniczak requested a review from a team as a code owner February 7, 2025 13:42
@weronikaolejniczak weronikaolejniczak force-pushed the fix/eui-plus-missing-content branch from ca0be91 to 2bc584d Compare February 7, 2025 13:45
@acstll acstll self-requested a review February 7, 2025 14:47
@acstll
Copy link
Contributor

acstll commented Feb 10, 2025

Here's what I found 🙂

EuiEmptyPrompt’s Guidelines subpage

  • in the "Empty state types, goals, and recommendations" section, some styles like the sidebar look different, also the template radio buttons seem overflowed [screenshot 1]
  • image comparisons don't have the same height, and in some cases the blueish background is missing (not sure if intended) [screenshot 2]
  • [not part of the changes] the "empty prompt usage guidelines" link in the Overview page that links to the Guidelines page is broken

[1]

Screenshot 2025-02-10 at 12 11 57

[2]

Old New
Screenshot 2025-02-10 at 12 47 06 old Screenshot 2025-02-10 at 12 47 18 new

EuiToast’s Guidelines subpage

  • in the examples, the "danger" button has a larger font size in all instances [screenshot 3]
  • [not part of the changes] the "toast usage guidelines" link in the Overview page that links to the Guidelines page is broken

[3]

Old New
Screenshot 2025-02-10 at 12 17 28 old Screenshot 2025-02-10 at 12 17 24 new

EuiTour’s Guidelines subpage

No comments 👍

Form controls’ Guidelines subpage

  • the comparison do/dont snippets don't have the same height, maybe it's intended but I think it looks better on the old site
  • in "Hint text" and "Placeholder text/In fields" sections there's a dont that should be do [screenshot 4]

[4]

Old New
Screenshot 2025-02-10 at 12 24 36 old Screenshot 2025-02-10 at 12 24 39 new

Selection controls’ Guidelines subpage

  • styles in the "Switch labels" do/dont section are strange [screenshot 5]

[5]

Old New
Screenshot 2025-02-10 at 12 27 53 old Screenshot 2025-02-10 at 12 28 00 new

Copy link
Contributor

@acstll acstll left a comment

Choose a reason for hiding this comment

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

Thank you @weronikaolejniczak for tackling this one!

The code looks perfect to me, but I found a few things looking at it from a user/designer perspective, I left them in this comment. Maybe some changes are intended, let me know.

@weronikaolejniczak
Copy link
Contributor Author

weronikaolejniczak commented Feb 12, 2025

I fixed whatever I could, some things I couldn't without going outside of the scope of this PR and I'd propose to fix them on another task 😄

EuiEmptyPrompt ❌

This one... 💀 With a bit of styling I managed to fix the "image radios" (5b34e63). The preview I was not able to get it to work as expected, hence #8312. Do you have an idea how to fix it?

I updated the link 👍🏻 (9052665)

Screen.Recording.2025-02-12.at.15.32.12.mov

EuiToast ✅

This one is super weird! 🤔 The danger buttons do have bigger font-size although the same size is passed. If you take a look at the Button page, it displays as expected there.

There are no additional styles passed to them. Something must be influencing it from higher up? Do you have an idea what could be wrong?

EDIT: @acstll found out that the issue is with Docusaurus applying paragraph styles to a text that is in newline. I created a ticket in M2 to come up with a meaningful fix for this: #8329

I updated the link 👍🏻 (0f0ae20)

Screenshots

Form controls ✅

I made the height of guideline panels equal and I changed some (incorrect) "don't" sections to "do" (925a1e4):

Screenshots

Selection controls ✅

I agree this table looks weird 👍🏻 What I found is that this is the responsive behavior of EuiBasicTable. The width of the page content is smaller than in the original docs, causing the table to pass a breakpoint and display like this. When I applied responsiveBreakpoint={false} it looks as in the original docs (8284ce5). I'm sure it's fine to get rid of the responsive behavior because it doesn't serve the purpose of this guideline specifically.

Screenshots

The code in: packages/website/docs/components/forms/selection_controls/guidelines.mdx is the same as in: packages/eui/src-docs/src/views/selection_controls/guidelines.js.

@acstll
Copy link
Contributor

acstll commented Feb 17, 2025

I fixed whatever I could, some things I couldn't without going outside of the scope of this PR and I'd propose to fix them on another task 😄

@weronikaolejniczak I agree we could fix these later on 👍

I'll take a quick look first at your comments for EuiEmptyPrompt and EuiToast first, and then re-run this locally for a final check 🤓

(btw the screenshots inside the <details> toggles are not rendering… maybe there's a space missing somewhere? putting markdown inside html does not always work, but I'm not sure that's the case…)

@weronikaolejniczak
Copy link
Contributor Author

@acstll ohhh, thanks for letting me know! 🙏🏻 I uploaded the screenshots first, put them into details and didn't bother to check if it works. Fixed by just using img. 🗒️ ✏️

Copy link
Contributor

@acstll acstll left a comment

Choose a reason for hiding this comment

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

1 (or 2 small things) more 🙏 🙏 🙏

@kibanamachine
Copy link

Preview staging links for this PR:

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @weronikaolejniczak

Copy link
Contributor

@acstll acstll left a comment

Choose a reason for hiding this comment

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

thank you very much @weronikaolejniczak 🙏

@weronikaolejniczak weronikaolejniczak linked an issue Feb 21, 2025 that may be closed by this pull request
5 tasks
@weronikaolejniczak weronikaolejniczak merged commit 623b75a into elastic:main Feb 21, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[EUI+] Missing guidelines content
4 participants