Skip to content

[ticket/14976] Convert all ems to px#4626

Merged
marc1706 merged 15 commits intophpbb:masterfrom
hanakin:ticket/14976
Oct 15, 2017
Merged

[ticket/14976] Convert all ems to px#4626
marc1706 merged 15 commits intophpbb:masterfrom
hanakin:ticket/14976

Conversation

@hanakin
Copy link
Member

@hanakin hanakin commented Jan 9, 2017

This should be the last major overhaul for standardization but the one that may require the largest review so I want to get it up early to allow ample time for critiques and bug fixes

to be merged after #4726, #4727

Checklist:

  • Correct branch: master for new features; 3.3
  • Tests pass
  • Code follows coding guidelines: [master / 3.3.x]
  • Commit follows commit message format
  • Browser tests completed [IE 11, Edge, FF, Chrome, Safari, IOS]

PHPBB3-14976

https://tracker.phpbb.com/browse/PHPBB3-14976

@hanakin hanakin changed the title Ticket/14976 [Ticket/14976] Convert all ems to px Jan 12, 2017
@hanakin hanakin changed the title [Ticket/14976] Convert all ems to px [ticket/14976] Convert all ems to px Mar 9, 2017
@hanakin
Copy link
Member Author

hanakin commented Jun 1, 2017

rdy for review

.nojs .tabs a span,
.nojs .minitabs a span {
letter-spacing: -0.5px;
letter-spacing: -0.031em;
Copy link
Member

Choose a reason for hiding this comment

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

Any reason why you changed this from px to em then?

Copy link
Member Author

Choose a reason for hiding this comment

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

letter-spacing and word-spacing are the only property exceptions to not using em as they have to be fractional which is not possible with px. It is also safe to use them for them as you never inherit this its always explicitly set at the child and not the parent

padding-right: 2px;
}

/* Not used anywhere */
Copy link
Member

Choose a reason for hiding this comment

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

Might be left over from previously when we had a different type of dropdown for the sort options.

Copy link
Member Author

Choose a reason for hiding this comment

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

@hanakin
Copy link
Member Author

hanakin commented Sep 7, 2017

Rdy!

@hanakin
Copy link
Member Author

hanakin commented Oct 11, 2017

@phpbb/development-team this is ready pending errors

@marc1706
Copy link
Member

Travis has provided you with exactly one error: https://travis-ci.org/phpbb/phpbb/jobs/286664050#L628

margin-left: 176px;
}

fieldset.fields1 dd {
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be cleaner to have the border-left-width definition in here instead of directly overwriting the margin-left for fieldset.fields1 dd from above it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah saw that will fix it tonight

@hanakin
Copy link
Member Author

hanakin commented Oct 15, 2017

@marc1706 should be good now

@marc1706 marc1706 merged commit 052b556 into phpbb:master Oct 15, 2017
@marc1706
Copy link
Member

"What does this button do?" 😃

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