Skip to content

Remove redundant properties from .embed-responsive#28062

Merged
MartijnCuppens merged 1 commit intomasterfrom
v4-dev-mc-embed-cleanup
May 29, 2019
Merged

Remove redundant properties from .embed-responsive#28062
MartijnCuppens merged 1 commit intomasterfrom
v4-dev-mc-embed-cleanup

Conversation

@MartijnCuppens
Copy link
Member

We copied the .embed-responsive code from somewhere else, but there are a lot of redundant properties in it:

  • No need to set display: block; on a <div>
  • No need to remove padding from a <div>
  • No need to set overflow: hidden; because the inner content is spread in .embed-responsive
  • No need for bottom: 0;, because we already have height: 100%;

@ysds
Copy link
Contributor

ysds commented Jan 17, 2019

I want to hear @necolas 's thoughts.
29e495f#diff-57ae112f09250f95da6f28dfff1d519f

@MartijnCuppens
Copy link
Member Author

I also wonder if this css:

.embed-responsive .embed-responsive-item,
.embed-responsive iframe,
.embed-responsive embed,
.embed-responsive object {
  ...
}

should be rewritten as:

.embed-responsive > * {
  ...
}

I know we better avoid the universal selector but I think for this case with the child combinator and the decrement of the number of selectors, it might be acceptable.

@mdo
Copy link
Member

mdo commented Jan 20, 2019

The biggest problems we had with selector performance issue we ever had was using them too much with attribute selectors for column classes. In these instances, I think it's acceptable. However, I'm curious if folks have other content within these elements? Does anything else accompany an embed other than the source?

@XhmikosR
Copy link
Member

I believe it's better to err on the safe side and be as explicit as possible IMO.

@XhmikosR XhmikosR changed the base branch from v4-dev to master February 19, 2019 15:35
@XhmikosR XhmikosR added v5 and removed v4 labels Feb 19, 2019
@ysds
Copy link
Contributor

ysds commented May 29, 2019

I confirmed the CSS are correct in theory, and tested in IE11, Edge, Firefox and Chrome. Let’s merge this.

@MartijnCuppens Would you fix the conflict?

@MartijnCuppens MartijnCuppens force-pushed the v4-dev-mc-embed-cleanup branch from 4c829b1 to 387ae84 Compare May 29, 2019 16:29
@MartijnCuppens
Copy link
Member Author

Done

@MartijnCuppens MartijnCuppens force-pushed the v4-dev-mc-embed-cleanup branch from 387ae84 to b9673d4 Compare May 29, 2019 16:40
@MartijnCuppens
Copy link
Member Author

This is a minor optimization, not worth backporting this to v4, it might break some edge cases.

@MartijnCuppens MartijnCuppens merged commit 14cb65f into master May 29, 2019
@MartijnCuppens MartijnCuppens deleted the v4-dev-mc-embed-cleanup branch May 29, 2019 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants