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

breadcrumb: simplify appearance, improve extensibility #32249

Merged
merged 6 commits into from Dec 1, 2020

Conversation

patrickhlauke
Copy link
Member

@patrickhlauke patrickhlauke commented Nov 24, 2020

@XhmikosR
Copy link
Member

Shouldn't we mention this change in migration?

@patrickhlauke
Copy link
Member Author

ah, good point @XhmikosR ... doh

@patrickhlauke
Copy link
Member Author

patrickhlauke commented Nov 24, 2020

also, i just ripped out the padding stuff, but should it perhaps go for an explicit padding: 0 ?

[edit: decided to just go for it]

@MartijnCuppens
Copy link
Member

We could nullify the variables instead of removing them.

https://markdotto.com/2019/08/30/null-sass-variables/

@patrickhlauke
Copy link
Member Author

sigh...well, whichever, somebody make a decision :)

@MartijnCuppens
Copy link
Member

If we set

$breadcrumb-padding-y:              0 !default;
$breadcrumb-padding-x:              0 !default;
$breadcrumb-bg:                     null !default;
$breadcrumb-border-radius:          null !default;

we'll still be able to adjust paddings,bg & radius with Sass variables and keep the result as is (in this PR).

@ffoodd
Copy link
Member

ffoodd commented Nov 24, 2020

Agreed, didn't think about this but it would even not be a breaking change I guess, since variables would still exist 👌

@patrickhlauke
Copy link
Member Author

Right, done ... think it'd still be a breaking change if we change the default styles like this under people's feet though, so keeping this as v5 only at the moment

v5.0.0-beta1 automation moved this from Inbox to Approved Nov 25, 2020
Copy link
Member

@ffoodd ffoodd left a comment

Choose a reason for hiding this comment

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

👌

@XhmikosR
Copy link
Member

Let's wait for another approval from @mdo please :)

@mdo
Copy link
Member

mdo commented Nov 29, 2020

Pushed some content changes and the CSS custom property mention in #32053 (comment) along with @ffoodd's recommendation of the Sass fallback.

@XhmikosR
Copy link
Member

XhmikosR commented Nov 30, 2020

Some notes:

  1. some further characters are escaped in the snippets EDIT: NVM, it's the double quotes character around the base64 data
  2. style="--bs-breadcrumb-divider: ;" doesn't seem to be valid. If we confirm it is valid, I could ask upstream for a fix. Using style="--bs-breadcrumb-divider: '';" seems to also work and it's valid

@XhmikosR XhmikosR changed the title Remove background, padding, border from breadcrumb container breadcrumb: simplify appearance, improve extensibility Nov 30, 2020
patrickhlauke and others added 4 commits November 30, 2020 12:12
- Add CSS custom property with fallback to Sass variable
- Update docs to mention the new CSS custom property
- Rewrite some of the docs to use divider instead of separator, and add some context here and there
@XhmikosR
Copy link
Member

XhmikosR commented Dec 1, 2020

@twbs/css-review so, does this still look good to merge?

Copy link
Member

@MartijnCuppens MartijnCuppens left a comment

Choose a reason for hiding this comment

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

Now it does

@XhmikosR XhmikosR merged commit bf3c4d0 into main Dec 1, 2020
v5.0.0-beta1 automation moved this from Approved to Done Dec 1, 2020
@XhmikosR XhmikosR deleted the breadcrumb-cleanup branch December 1, 2020 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
v5.0.0-beta1
  
Done
Development

Successfully merging this pull request may close these issues.

Breadcrumbs text and default links fail contrast ratio
5 participants