Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix ?large to also control size of dialog #156

Merged
merged 3 commits into from
Mar 8, 2016

Conversation

laughinghan
Copy link
Contributor

Passed along from ?large to control the badge size, they now also
control the iframe and dialog size because after all, if you're on a
page with larger font-size that demands a larger badge, the small dialog
with small text is gonna look out of place:
screenshot of small dialog

The large badge is about 1.5x the size of the small badge (30px to 20px;
the other ratios aren't as exact), but I found that making the large
dialog 1.4x the size of the small dialog looked better than 1.5x
(that is, bumping the em/rem unit font size from 10px to 14px rather
than 15px), I think because at 1.5x the text in the dialog becomes bigger
than the text in the badge:

font-size: 14px
screenshot of 1.4x
vs
font-size: 15px
screenshot of 1.5x

See also 6108430 for discussion of the em/rem technique

Converting every dimension/length in all dialog styling to be in terms
of a 10px font size (chosen to be easy: 250px => 25em, 4px => .4em) makes
it easy to adjust the overall size of things by changing just that one
base font size unit that every dimension and length is in terms of.
For example, a one-line change of adjusting the base font size unit to
15px would scale everything up by a factor of 150%.

For the div, we can simply set the fontSize and convert all lengths from
px to em. Since the div is mostly just a container for the iframe, it
has few descendants and none override the fontSize (which is important
because em wouldn't be 10px on such a descendant).

For the splash page in the iframe, several elements do override the
font size, so instead we set the font-size of the root element ('html')
and convert all lengths from px to rem, aka "root em". Browser support
for rem is pretty good, no IE8 but IE9+ and all modern browsers:
  http://caniuse.com/rem
The one browser issue to deal with is that in IE9-10, rem doesn't work
in the 'font' CSS shorthand property (documented on "Can I use" and also
  http://stackoverflow.com/q/16157342/362030
and
  http://codersblock.com/blog/font-shorthand-bug-in-ie10/
) but the fix is pretty trivial.
Passed along from ?large to control the badge size, they now also
control the iframe and dialog size because after all, if you're on a
page with larger font-size that demands a larger badge, the small dialog
with small text is gonna look out of place:  https://git.io/v2Xy4

The large badge is about 1.5x the size of the small badge (30px to 20px;
the other ratios aren't as exact), but I found that making the large
dialog 1.4x the size of the small dialog looked better than 1.5x
(that is, bumping the em/rem unit font size from 10px to 14px rather
than 15px):

font-size: 14px  https://git.io/v2XDh
vs
font-size: 15px  https://git.io/v2XyJ
@rauchg
Copy link
Owner

rauchg commented Mar 4, 2016

Please rebase. I like this. I don't like the empty space at the bottom as much

Conflicts with rauchg#153 feature.sign-in-link-in-iframe:
	lib/index.js
	  Added `large` param to splash() call, to which rauchg#153 had added `org`
	lib/splash.js
	  Converted px to rem where rauchg#153 had dedented
@laughinghan
Copy link
Contributor Author

@rauchg Done!

@rauchg
Copy link
Owner

rauchg commented Mar 8, 2016

@laughinghan is the empty space still there?

@laughinghan
Copy link
Contributor Author

@rauchg
Nope, thanks to #153:
image

@rauchg
Copy link
Owner

rauchg commented Mar 8, 2016

Amazing. Thank you so much.

rauchg added a commit that referenced this pull request Mar 8, 2016
Fix ?large to also control size of dialog
@rauchg rauchg merged commit 7e23329 into rauchg:master Mar 8, 2016
@laughinghan laughinghan deleted the fix.large-dialogs branch March 8, 2016 21:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants