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

Add stylistic tweaks to the alt show page template #36

Merged
merged 4 commits into from
Jan 19, 2017
Merged

Conversation

marineb
Copy link
Contributor

@marineb marineb commented Jan 18, 2017

No description provided.

@marineb marineb requested a review from noslouch January 18, 2017 23:30
@marineb marineb changed the title Add a nice gradient on the navigation so people know they can scroll Add stylistic tweaks to the alt show page template Jan 19, 2017
background: linear-gradient(90deg, rgba(255, 255, 255, 0),rgba(255, 255, 255,1));
position: absolute;
right: 0;
top: 5px;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why isn't this height: 100% and top: 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👌 I just made it taller instead.

.flag {
display: flex;
align-items: center;
justify-content: center;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you use the mixin instead of raw values? The mixin provides for crossbrowser compat

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, assumed our pre-processor was doing that. Changing!

&:after {
content: " ";
width: 25px;
height: 40px;
Copy link
Contributor

Choose a reason for hiding this comment

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

I see you changed it, but specifically shouldn't this be 100% so that if the height of the list changes (maybe we use a bigger font size), this thing will adjust accordingly?

.flag {
@include display-flex;
@include align-items(center);
@include justify-content(center);
Copy link
Contributor

Choose a reason for hiding this comment

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

use the flexbox mixin like so:

@include flexbox((
  display: flex,
  align-items: center,
  justify-content: center
));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I saw it used like I put it in other places. Will change!

Copy link
Contributor

@noslouch noslouch left a comment

Choose a reason for hiding this comment

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

👍

@marineb marineb merged commit 94ecb45 into master Jan 19, 2017
@noslouch noslouch deleted the alt-channel branch January 23, 2017 23:26
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