Theme: removed or changed occurrences of zoom #852

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
3 participants
@selfthinker
Contributor

selfthinker commented Nov 29, 2012

Fixes #7975 - Remove invalid CSS for legacy browsers

Because of overlapping issues, this also reverts most of e77edc6 and fixes it in a different way. The original commit tried to fix it in core while the real issue was actually in the demo.

Theme: removed or changed occurrences of zoom. Fixes #7975 - Remove i…
…nvalid CSS for legacy browsers

Because of overlapping issues, this also reverts most of e77edc6 and fixes it in a different way.
@scottgonzalez

This comment has been minimized.

Show comment Hide comment
@scottgonzalez

scottgonzalez Nov 29, 2012

Owner

Thanks, can you just change the for IE7 comments to support: IE7? We use the support: format to document all browser-specific code so that it's easy to find when changing our level of support.

Owner

scottgonzalez commented Nov 29, 2012

Thanks, can you just change the for IE7 comments to support: IE7? We use the support: format to document all browser-specific code so that it's easy to find when changing our level of support.

demos/button/toolbar.html
- *:first-child+html #toolbar {
- padding: 4px 0px 4px 5px;
+ padding: 4px;
+ display: inline-block;

This comment has been minimized.

Show comment Hide comment
@mikesherov

mikesherov Nov 29, 2012

Member

@selfthinker, I'm guessing you checked, how do these changes effect the toolbar demo in IE7. I literally just landed this so I want to be sure :-)

@mikesherov

mikesherov Nov 29, 2012

Member

@selfthinker, I'm guessing you checked, how do these changes effect the toolbar demo in IE7. I literally just landed this so I want to be sure :-)

This comment has been minimized.

Show comment Hide comment
@selfthinker

selfthinker Nov 29, 2012

Contributor

Yes, I've checked and my changes in toolbar.html fixes the same main issue that the toolbar background looked all weird.
Although the original code "fixed" one more issue (in the button css), i.e. that the buttons were not lined up properly, it also introduced a new bug that made every <button> with more than 1 word unusable (e.g. the button in the split button demo was running over 3 lines).

@selfthinker

selfthinker Nov 29, 2012

Contributor

Yes, I've checked and my changes in toolbar.html fixes the same main issue that the toolbar background looked all weird.
Although the original code "fixed" one more issue (in the button css), i.e. that the buttons were not lined up properly, it also introduced a new bug that made every <button> with more than 1 word unusable (e.g. the button in the split button demo was running over 3 lines).

This comment has been minimized.

Show comment Hide comment
@selfthinker

selfthinker Nov 29, 2012

Contributor

A further explanation what was the problem in the first place: The #toolbar is a span (for whatever reason) and therefore top and bottom paddings won't behave properly. By making it have a display of either "block" or "inline-block", it can properly contain its elements and can/should take the same top and bottom padding as the left and right padding.
(Ideally using a div instead of a span would make much more sense anyway.)

I have no idea why the padding of the #toolbar was changed for IE7. In all browsers there is a bit more space on the right than on the left, because every button has a right margin. In all IE versions it seems to be a bit more, but IE7 is not different to other IEs. Maybe it was before my fix, though.

If you'd like I can make screenshots before and after the fixes.

@selfthinker

selfthinker Nov 29, 2012

Contributor

A further explanation what was the problem in the first place: The #toolbar is a span (for whatever reason) and therefore top and bottom paddings won't behave properly. By making it have a display of either "block" or "inline-block", it can properly contain its elements and can/should take the same top and bottom padding as the left and right padding.
(Ideally using a div instead of a span would make much more sense anyway.)

I have no idea why the padding of the #toolbar was changed for IE7. In all browsers there is a bit more space on the right than on the left, because every button has a right margin. In all IE versions it seems to be a bit more, but IE7 is not different to other IEs. Maybe it was before my fix, though.

If you'd like I can make screenshots before and after the fixes.

This comment has been minimized.

Show comment Hide comment
@scottgonzalez

scottgonzalez Nov 29, 2012

Owner

I'm not sure why it's a span either. We can certainly change it to a div if it'll help with styling.

@scottgonzalez

scottgonzalez Nov 29, 2012

Owner

I'm not sure why it's a span either. We can certainly change it to a div if it'll help with styling.

This comment has been minimized.

Show comment Hide comment
@selfthinker

selfthinker Nov 29, 2012

Contributor

It's not necessary for the styling as the display: inline-block; works the same for both divs and spans. It's just if it had been a div, it wouldn't have needed it in the first place. But a div makes much more sense for many other reasons.

@selfthinker

selfthinker Nov 29, 2012

Contributor

It's not necessary for the styling as the display: inline-block; works the same for both divs and spans. It's just if it had been a div, it wouldn't have needed it in the first place. But a div makes much more sense for many other reasons.

@mikesherov

This comment has been minimized.

Show comment Hide comment
@mikesherov

mikesherov Nov 30, 2012

Member

@selfthinker, thanks for following up here. this needs to be landed along with gh-851?

Member

mikesherov commented Nov 30, 2012

@selfthinker, thanks for following up here. this needs to be landed along with gh-851?

@selfthinker

This comment has been minimized.

Show comment Hide comment
@selfthinker

selfthinker Nov 30, 2012

Contributor

Well, this and the other PR started out as two different things. But the fix for the toolbar demo got in between and concerns both. They don't necessarily need to land together. If they don't or the other comes first, the only downside is what I commented on gh-851 (i.e. toolbar spacing will look unbalanced).

Contributor

selfthinker commented Nov 30, 2012

Well, this and the other PR started out as two different things. But the fix for the toolbar demo got in between and concerns both. They don't necessarily need to land together. If they don't or the other comes first, the only downside is what I commented on gh-851 (i.e. toolbar spacing will look unbalanced).

@mikesherov

This comment has been minimized.

Show comment Hide comment
@mikesherov

mikesherov Nov 30, 2012

Member

Ok, I'm going to land this then. Thanks!

Member

mikesherov commented Nov 30, 2012

Ok, I'm going to land this then. Thanks!

@mikesherov

This comment has been minimized.

Show comment Hide comment
@mikesherov

mikesherov Nov 30, 2012

Member

Thanks, landed in d7bff01. Can you please check to make sure I merged correctly, @selfthinker?

Member

mikesherov commented Nov 30, 2012

Thanks, landed in d7bff01. Can you please check to make sure I merged correctly, @selfthinker?

@mikesherov mikesherov closed this Nov 30, 2012

@selfthinker

This comment has been minimized.

Show comment Hide comment
@selfthinker

selfthinker Nov 30, 2012

Contributor

Yup, looking good. Thanks!

Contributor

selfthinker commented Nov 30, 2012

Yup, looking good. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment