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 class for images with a drop shadow #514

Merged
merged 5 commits into from
Mar 23, 2021

Conversation

tobyhodges
Copy link
Member

To address the issue described in #513, this introduces a img.image-with-shadow class with a drop shadow. If and when this is accepted, I'll document it in the lesson example.

assets/css/lesson.scss Outdated Show resolved Hide resolved
@maxim-belkin
Copy link
Contributor

@tobyhodges, would you mind updating the PR to have a single drop shadow only? Or is there a case where two similar shadows are useful? Also, what do you think about changing img.image-with-shadow to article img ?

@maxim-belkin maxim-belkin added status:waiting for response Waiting for Contributor to respond to maintainers' comments or update PR type:template and tools Issue about template and tools labels Mar 16, 2021
@tobyhodges
Copy link
Member Author

This fell off my radar, sorry. I have made a note to update the branch after the Core Team retreat is over (next week)

@tobyhodges
Copy link
Member Author

@maxim-belkin updated to use a single, grey shadow as you proposed. I did not adjust the selector to apply this shadow to all images in the article because I suspect the shadow effect will be undesirable in many circumstances. IMO it is better as an "opt-in" feature, which the .image-with-shadow class will provide.

@tobyhodges tobyhodges requested a review from maxim-belkin March 22, 2021 10:43
@maxim-belkin
Copy link
Contributor

Thanks, Toby. Looks good. Current selector works when used like so:

![alt text](../fig/figure.png){: .image-with-shadow}
![alt text](../fig/figure2.png){: class="image-with-shadow"}

I have to admit that in my first attempt I did:

![alt text](../fig/figure.png)
{: .image-with-shadow}

which resulted in the following HTML structure: p[class="image-with-shadow"] > img. I know this is my mistake but I wonder if this is something we want to take care of. All we'd have to do is extend the selector to images within paragraphs with "image-with-shadow" class:

p.image-with-shadow img,
img.image-with-shadow { ...

@tobyhodges
Copy link
Member Author

That is a great suggestion, @maxim-belkin, because I agree that many lesson authors are likely to try what you tried (it is also what my muscle memory tries to make me do every time!)

@tobyhodges
Copy link
Member Author

Addressed in 4b20368

Copy link
Contributor

@maxim-belkin maxim-belkin left a comment

Choose a reason for hiding this comment

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

Looks great! 👍🏻

@maxim-belkin
Copy link
Contributor

Thank you, Toby!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:waiting for response Waiting for Contributor to respond to maintainers' comments or update PR type:template and tools Issue about template and tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants