Automattic / notifications-panel

Core notifications panel for WordPress.com notifications
0 stars 1 forks source link

Styling: Center all images by default #196

Closed dmsnell closed 6 years ago

dmsnell commented 6 years ago

A couple times recently we have seen attempts to center images through styles embedded in notification content directly or by using a "badge" type notification. The problem with these approaches is that they end up working around the system in the notifications panel and become fragile and coupled to things that are implementation details instead of design elements.

After discussion with a few people we realized that there is no reason for us not to automatically center images displayed in the notifications themselves. In fact, the mobile clients already do this.

In this patch we're centering all images inside of the notifications. This should make it possible to do what people have wanted, which is to have a left-aligned paragraph with a centered image which is also not a badge/trophy notification.

Testing

You'll need to generate notifications with images inside of them in order to test this. There are two or three primary circumstances I think we want to test:

Some images will come as floating pieces inside of a post, for example. It would be best to test these with a mention notification maybe as it will pull in post contents. Some will have the images as their own blocks found between paragraphs of text.

Just try to see if this breaks anything. It would be helpful to resize the screen while testing to make sure the transforms don't break.

Notes

As always, my CSS skills are a bit -weak-sauce: so please don't hesitate to critique the rules and offer a better alternative. I wrote what gave me my desired result, not what I necessarily think is most appropriate.

Before

screen shot 2017-11-15 at 3 40 22 pm

After

screen shot 2017-11-15 at 3 39 53 pm

From the screenshots you can see that the max-width rule makes some images expand.

davewhitley commented 6 years ago

Curious that the little cat image does not expand full width like the black cat image does. I'm wondering why the images need to expand full width. Unless, in this case, it seems that the black cat image is probably larger than the panel width.

No objections

dmsnell commented 6 years ago

Curious that the little cat image does not expand full width like the black cat image does. I'm wondering why the images need to expand full width. Unless, in this case, it seems that the black cat image is probably larger than the panel width.

yeah @drw158 the cross-stitch is actually wider than the notes panel while the small cat is only 64x64. I'm actually more curious why the cross-stitch didn't already expand unless Safari scaled it down for some 2x effect

kwight commented 6 years ago

Any reasons against more straightforward CSS?

.wpnc__image {
    display: block;
    margin: auto;
    max-width: 100%;
}
dmsnell commented 6 years ago

Any reasons against more straightforward CSS?

Works for me. Updated and tested in 420688f