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

EM-1570: Product Feature Modal (ESB) #119

Merged
merged 3 commits into from
Aug 4, 2017

Conversation

mrfantasticwonders
Copy link
Contributor

@mrfantasticwonders mrfantasticwonders commented Jul 31, 2017

Purpose:

As a visitor to a page, when I see a product's features that has more info, I can click on a 'read more link' that opens a modal that allows me to see a section for imagery and a section for copy that lets me know more about a specific feature.

This PR does the following:

  • Adds in variables controlling the product feature modal
  • Adds in a theme for the default close button for our modals

Visit the link for more information about the modal:
https://github.com/cbdr/employer/pull/949

Copy link
Contributor

@arelia arelia left a comment

Choose a reason for hiding this comment

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

maybe just change the color variable, I don't know whether or not I'm right about the other two things

@@ -31,3 +31,16 @@
text-decoration: none;
}
}

@mixin close-x-overflow {
width: 25px;
Copy link
Contributor

Choose a reason for hiding this comment

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

should these values be $feature-modal-controls-size? (only if they're meant to be the same size rather than they just happen to be the same 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.

I'm going to create another variable for this. The two are unrelated however despite being the same size.

width: 25px;
height: 25px;
border-radius: 50%;
background-color: #333;
Copy link
Contributor

Choose a reason for hiding this comment

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

$grey-darkest

color: white;
line-height: 0;
font-size: 16px;
right: -10px;
Copy link
Contributor

Choose a reason for hiding this comment

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

looking at the redlines doc, does the corner meet right in the middle of the circle? (i'm not entirely sure, you probably reviewed this with Susan before she left) If that's the case and if you use a variable for the height and width of the circle, you could use -$feature-modal-controls-size / 2 here

@@ -61,12 +61,18 @@ $search__thumbnail-height: 5.3125rem; // 85px
$sitemap-angle-icon: 17px;
$social-media-icons: 19.2px;
$toggle-switch-width: 65px;
$visual-divider-endpoint-size: 13px;
$visual-divider-endpoint-size: 13px;
Copy link
Contributor

Choose a reason for hiding this comment

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

there was an extra space added at the end here

…f the close button for the prod modal, updating some colors
@mrfantasticwonders
Copy link
Contributor Author

@arelia Feedback has been addressed, this PR is ready for your review again.

@mrfantasticwonders mrfantasticwonders merged commit 9c64e7d into master Aug 4, 2017
@mrfantasticwonders mrfantasticwonders deleted the topic/EM-1570-ProdFeatureModal branch August 4, 2017 15:11
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.

4 participants