Skip to content
This repository has been archived by the owner on Jan 17, 2023. It is now read-only.

Fixes #5002 - UI fixes in non-Firefox #5071

Merged

Conversation

punamdahiya
Copy link
Contributor

No description provided.

@punamdahiya
Copy link
Contributor Author

punamdahiya commented Oct 26, 2018

Fixes #5072

// Depending on the user agent computed styles
// line-height can have value as normal
if (line_height.toLowerCase() === "normal") {
line_height = 1.1 * FONT_SIZE;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using value 1.1 (approximate default 1.2) to compute line-height for the fonts used in TextTool

Copy link
Member

Choose a reason for hiding this comment

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

Hmm. This special case doesn't cover other weird stuff, like ems or percentages, that might be set as a user agent's default. Can we just set the line-height in the CSS directly?

@punamdahiya
Copy link
Contributor Author

Fixes #5002

@punamdahiya
Copy link
Contributor Author

Fixes #5106

Copy link
Member

@jaredhirsch jaredhirsch left a comment

Choose a reason for hiding this comment

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

Not a huge deal, but it's better to have separate PRs for small fixes, because some bugs are fixed here, and some still aren't.

OK, bad news first:


Seems related to #5002 item 1, the 404 page layout in this branch is pretty broken on safari and chrome:

screen shot 2018-10-30 at 1 59 14 pm


I'm also seeing a stray backtick in the shots page on safari:

screen shot 2018-10-30 at 2 11 34 pm


Stuff that's fixed:

screen shot 2018-10-30 at 1 49 40 pm

screen shot 2018-10-30 at 1 57 28 pm

@punamdahiya
Copy link
Contributor Author

punamdahiya commented Oct 30, 2018

Page not found issue got fixed in master with #5034, I will rebase PR on top of latest master, that should fix the issue

@punamdahiya punamdahiya force-pushed the 5002_Non-Firefox-UI-Fixes branch from 079443d to 9d551bf Compare October 30, 2018 21:38
@punamdahiya
Copy link
Contributor Author

@6a68 Updated PR with master that should fix page not found, thanks for catching the stray tick updated to remove that in shot index view.

Copy link
Member

@jaredhirsch jaredhirsch left a comment

Choose a reason for hiding this comment

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

Looks good, one question about overriding the line-height defaults to simplify things

// Depending on the user agent computed styles
// line-height can have value as normal
if (line_height.toLowerCase() === "normal") {
line_height = 1.1 * FONT_SIZE;
Copy link
Member

Choose a reason for hiding this comment

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

Hmm. This special case doesn't cover other weird stuff, like ems or percentages, that might be set as a user agent's default. Can we just set the line-height in the CSS directly?

@punamdahiya
Copy link
Contributor Author

punamdahiya commented Oct 31, 2018

@6a68 AFAIK It’s hard to determine line-height for a font-size and set in CSS as it varies across platform.

Also, as per w3c input default line-height in all user agents should be normal.

@jaredhirsch
Copy link
Member

Heh, thanks for schooling me. I wasn't thinking that you wrote the text tool, and have bumped into all these edge cases already.

@jaredhirsch jaredhirsch merged commit 9fb24f1 into mozilla-services:master Oct 31, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants