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

jekyll(css): add experimental note #15223

Merged
merged 2 commits into from
Jul 27, 2022
Merged

Conversation

crazy-max
Copy link
Member

While working on #15194, I found out the "note" block design was not really welcoming for new users:

image

This PR adds a new note type called experimental that will look like this:

image

image

Also needs to upgrade Font Awesome because the vial icon is missing with the current release we have.

Not that I moved the Font Awesome library to an assets directory because accessing the resources from their css moved the fonts to a webfonts folder which is relative to their css. I didn't want to add another folder to root path of this repo as it's already bloated and I think it makes more sense to have the resources dedicated to the libraries in their respective folders so you know which css is linked to which font (same for javascript).

I think we should also do the same for other "assets" we have in css, js, favicons, fonts folders and move these into the assets folder in a follow-up.

Maybe also in the future use npm and a package.json to manage these dependencies. (cc @mark-dr)

@netlify
Copy link

netlify bot commented Jul 27, 2022

Deploy Preview for docsdocker ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit b672b28
🔍 Latest deploy log https://app.netlify.com/sites/docsdocker/deploys/62e145a17a4592000875afe1
😎 Deploy Preview https://deploy-preview-15223--docsdocker.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@crazy-max crazy-max force-pushed the note-experimental branch 2 times, most recently from e1bfd62 to 17bc30a Compare July 27, 2022 10:37
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM, I like it 👍

@thaJeztah
Copy link
Member

I recall I did some testing for redesign these in #11641 as well

@crazy-max
Copy link
Member Author

I recall I did some testing for redesign these in #11641 as well

Ah looks neat! I'm using admonition on personal repos and I think they look great: https://squidfunk.github.io/mkdocs-material/reference/admonitions/#supported-types

image

@thaJeztah
Copy link
Member

Yeah, my intent with that PR was to have a clear "box" for these (as some are quite lengthy), without being too much separate from the content (i.e., not a "in your face" background color, so that we also don't have to re-invent colour-schemes that "work" for each and every one of them). Also to make sure it's all just CSS, and not too much HTML markup needed.

@usha-mandya
Copy link
Member

@mark-dr Could you PTAL?

@usha-mandya
Copy link
Member

Thanks @crazy-max. Going forward, we need to use {:.experimental} to use the new style for experimental commands. Is this correct? Also, is there anything else we should do to get this style? TIA

@crazy-max
Copy link
Member Author

Thanks @crazy-max. Going forward, we need to use {:.experimental} to use the new style for experimental commands. Is this correct? Also, is there anything else we should do to get this style? TIA

yes indeed, see docker/buildx#1236

@mark-dr
Copy link
Contributor

mark-dr commented Jul 27, 2022

The changes look good to me on a technical level, but we should be careful about Docs drifting away from the design system. Our existing alerts are based on the Alerts in this figma doc:

https://www.figma.com/file/R0GyrsN8tRxJIbti0KY6S3/%E2%9C%A8Components-%26-Patterns?node-id=2%3A8

In there, we use green to indicate success (i.e. the thing you just attempted was successful). Maybe this should be an info alert, or maybe even a "special" purple one. WDYT, @MCallaghanDocker?

@mark-dr
Copy link
Contributor

mark-dr commented Jul 27, 2022

See conversation here - it's currently leaning toward blue being the right colour for this.

https://docker.slack.com/archives/C0324PUQMFF/p1658928599667639

@crazy-max
Copy link
Member Author

crazy-max commented Jul 27, 2022

we should be careful about Docs drifting away from the design system. Our existing alerts are based on the Alerts in this figma doc:

https://www.figma.com/file/R0GyrsN8tRxJIbti0KY6S3/%E2%9C%A8Components-%26-Patterns?node-id=2%3A8

Of course! Let me know what would work best for us.

In there, we use green to indicate success (i.e. the thing you just attempted was successful). Maybe this should be an info alert

IMO there are two patterns to take into account. There is callouts and actionable/story states. Callouts being passive like this "experimental" note and "success" for actionable ones or describing a story. But ok to switch to an info alert if it follows our best practices 👍

or maybe even a "special" purple one.

This one LGTM too.

@mark-dr
Copy link
Contributor

mark-dr commented Jul 27, 2022

@crazy-max, how about blue (i.e. the "info" colour), with the vial?

@crazy-max
Copy link
Member Author

@crazy-max, how about blue (i.e. the "info" colour), with the vial?

Yes sgtm!

@crazy-max
Copy link
Member Author

@mark-dr Here it is using the same palette as current info note.

image

image

@crazy-max crazy-max force-pushed the note-experimental branch from 17bc30a to b672b28 Compare July 27, 2022 14:03
Copy link
Contributor

@mark-dr mark-dr left a comment

Choose a reason for hiding this comment

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

Thanks for the color swap, looks good!

@crazy-max
Copy link
Member Author

Ok let's get this one in if ci ok

@crazy-max crazy-max enabled auto-merge July 27, 2022 14:06
@crazy-max crazy-max merged commit fba3160 into docker:master Jul 27, 2022
@crazy-max crazy-max deleted the note-experimental branch July 27, 2022 14:07
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.

4 participants