Skip to content

Fix incorrect variable for popover border radius#28733

Merged
MartijnCuppens merged 2 commits into
twbs:masterfrom
ysds:master-ysds-popover-border-radius
May 7, 2019
Merged

Fix incorrect variable for popover border radius#28733
MartijnCuppens merged 2 commits into
twbs:masterfrom
ysds:master-ysds-popover-border-radius

Conversation

@ysds

@ysds ysds commented May 6, 2019

Copy link
Copy Markdown
Contributor

Use $popover-border-radius variable instead of $border-radius-lg.

@ysds ysds requested a review from a team as a code owner May 6, 2019 15:29
@ysds

ysds commented May 6, 2019

Copy link
Copy Markdown
Contributor Author

I noticed that the _popover.scss uses the local scope $offset-border-width variable.

Should we add $popover-inner-border-radius to _variable.scss like the $card-inner-border-radius for consistency?

But I like local scope because there is no case to customize those variables.

@MartijnCuppens

Copy link
Copy Markdown
Member

Should we add $popover-inner-border-radius to _variable.scss like the $card-inner-border-radius for consistency?

Yup

But I like local scope because there is no case to customize those variables.

But disables the ability to set the radius to null for example. I also know there are some issues with calc() and width of 0 which can be avoided by moving this variable to _variables.scss

@ysds

ysds commented May 7, 2019

Copy link
Copy Markdown
Contributor Author

Done :)

@MartijnCuppens MartijnCuppens merged commit dfab49a into twbs:master May 7, 2019
@ysds ysds deleted the master-ysds-popover-border-radius branch May 7, 2019 10:12
XhmikosR pushed a commit that referenced this pull request May 7, 2019
* Use $popover-border-radius

* Move and rename the local $offset-border-width to the global variable
@luckylichi luckylichi mentioned this pull request May 7, 2019
XhmikosR pushed a commit that referenced this pull request May 9, 2019
* Use $popover-border-radius

* Move and rename the local $offset-border-width to the global variable
@mdo mdo mentioned this pull request Jul 22, 2019
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.

3 participants