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

Blog layout enhancements #581

Merged
merged 11 commits into from
Sep 20, 2021
Merged

Blog layout enhancements #581

merged 11 commits into from
Sep 20, 2021

Conversation

kmeleta
Copy link
Contributor

@kmeleta kmeleta commented Sep 7, 2021

Why are these changes introduced?

Fixes #418

  • Add desktop layout setting to main blog
  • Add image height setting to article and article cards (main blog and featured blog)
  • Prevent single blog post from going width (on large screens) in featured blog section

What approach did you take?

Explicit image heights were added for each breakpoint/setting combination according to the design. If we were concerned about the number of lines of css this adds (and we didn't mind the heights being less fixed across viewport sizes), we may be able to use percentage based heights that were shared across at least some of the breakpoints. This could save some css.

Demo links

Checklist

@ghost ghost added the cla-needed label Sep 7, 2021
@ghost ghost removed the cla-needed label Sep 7, 2021
@kmeleta kmeleta marked this pull request as ready for review September 7, 2021 19:47
Oliviammarcello
Oliviammarcello previously approved these changes Sep 8, 2021
Copy link
Contributor

@Oliviammarcello Oliviammarcello left a comment

Choose a reason for hiding this comment

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

This looks great! I have no notes 🎉

@ludoboludo ludoboludo self-requested a review September 8, 2021 13:49
Copy link
Contributor

@ludoboludo ludoboludo left a comment

Choose a reason for hiding this comment

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

Left a few comments.

One thing not related that looks like it could be maybe be a quick fix 😬 is the share button being very wide on the article page. Unless it's fixed already on main.

assets/component-article-card.css Show resolved Hide resolved
Comment on lines -82 to -91
@media screen and (min-width: 750px) {
.blog__post:only-child {
text-align: center;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should still try and center the blog card when it's on its own 🤔 If you have only one article or decide to limit it to one, it mostly looks like it's missing a second one rather than looking ok alone.

cc: @Oliviammarcello

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely a design decision but I don't know if we have a precedent for centering a card like that anywhere, unless full width. Centering might make it look more purposeful in some way, but I wouldn't say I'm crazy about it, personally https://screenshot.click/08-27-azpow-eji6o.png

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair it looks odd with the title of the section left aligned. Disregard my comment 👍

Choose a reason for hiding this comment

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

This is a fair point, but I think it's fine to keep the card left aligned when there's only 1 post. We could argue that this section has less purpose when a blog has less than 2 articles in it. And also, this layout is the same for the featured collection when it only has one product. About that, I tested it and when I resize the browser window with those to sections, the alignment seems off on the tablet breakpoint. Is that intentional? If not I would suggest we investigate the issue and make the proper update. Here's a video recording to show what I mean.

And then there's another question that deserves to be asked on the UX side of things. Should we change the range picker start and stop values in the Blog post section settings? Since 1 is not the use case we designed it for, perhaps it could start at 2 ? And then I'm wondering why we don't allow to go up to 4. You may remember @Oliviammarcello if there's a rational I'm forgetting?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the with the padding and spacing we had, the line length on the cards got a bit too short at 4

Copy link
Contributor

@Oliviammarcello Oliviammarcello Sep 8, 2021

Choose a reason for hiding this comment

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

Now that the margins are wider maybe that's no longer a problem? 🤔 @kmeleta would we be able to try adding 4?

Choose a reason for hiding this comment

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

I believe the with the padding and spacing we had, the line length on the cards got a bit too short at 4

Gotcha. I think I remember having discussed that. Makes total sense. 👍

Copy link
Contributor Author

@kmeleta kmeleta Sep 8, 2021

Choose a reason for hiding this comment

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

This is quick mockup of what a 4 column blog looks like at the smallest desktop breakpoint (~990px) with all the current desktop paddings/sizes/etc. The titles seem a little squeezed to me in this case, but 4 articles on larger desktop sizes looks fine, and 4 articles on tablet or mobile is fine because they're just additional slides. If that looks reasonable enough I can definitely make that change. Let me know what you think.

Guillaume I think that tablet alignment you noticed is due to the tablet slider margins being applied even when it's not actually a slider. This should occur on tablet, when only 2 articles are displayed. I'll see if I can conditionally prevent that.

Copy link
Contributor

Choose a reason for hiding this comment

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

@kmeleta I'm in favour of adding this in 🙌 Just to clarify, the blog posts slider will now start at 2 and have 3 in the middle and 4 on the right side.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The limit slider has been updated to support 2-4 and I fixed the padding discrepancies on the tablet breakpoint when the slider is/isn't active. I also realized I originally forgot to update the placeholder card width.

.blog-articles > *:nth-child(4),
.blog-articles > *:last-child:nth-child(2),
.blog-articles > *:last-child:nth-child(5) {
.blog-articles--collage > *:nth-child(3n + 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

With this change, .blog-articles > *:last-child:nth-child(2) and .blog-articles > *:last-child:nth-child(5) aren't handled like they were before anymore: screenshot

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I guess reading the ticket again, this change isn't called out for collage layout specifically, only grid. So you're probably right I may have to put this back in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I think I found a way to get those cases accounted for again without having to explicitly add a selector for 2 and 5 (or 1 and 4), which would really add up if being repeated for each size/breakpoint.

"label": "t:sections.main-blog.settings.image_height.options__4.label"
}
],
"default": "adapt",
Copy link
Contributor

Choose a reason for hiding this comment

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

default, based on the image shared by Guillaume in the issue should be small. The Adapt to image could be moved to the bottom as the last option to match that screenshot as well 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In Figma currently Adapt is on top, but it wasn't clear what was intended to be default. Figured there was a chance we'd want to play with these settings. Thoughts order and default @Oliviammarcello? Did I miss this decision somewhere?

Copy link
Contributor

@Oliviammarcello Oliviammarcello Sep 8, 2021

Choose a reason for hiding this comment

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

Sorry for the confusion. Let's use place Adapt at the top (it's at the top for other image settings). But let's use small as the default still 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now defaulting to small

@nicklepine
Copy link

@shainaraskas Pinging you on this for visibility as this change will also require small updates to help docs.

@shainaraskas
Copy link

@KimliW 🙏

Copy link
Contributor

@ludoboludo ludoboludo left a comment

Choose a reason for hiding this comment

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

Left a couple comments but it's looking good 👌

assets/base.css Outdated Show resolved Hide resolved
assets/base.css Outdated Show resolved Hide resolved
snippets/article-card.liquid Show resolved Hide resolved
@ludoboludo
Copy link
Contributor

I just noticed that if you have a short excerpt or article content, it centers the text. So maybe we can make sure it's left aligned:

ludoboludo
ludoboludo previously approved these changes Sep 15, 2021
Copy link
Contributor

@ludoboludo ludoboludo left a comment

Choose a reason for hiding this comment

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

It's looking good to me 👌

I requested the translations while we wait for a second reviewer 🌐

@tauthomas01 tauthomas01 self-assigned this Sep 15, 2021
tauthomas01
tauthomas01 previously approved these changes Sep 15, 2021
Copy link
Contributor

@tauthomas01 tauthomas01 left a comment

Choose a reason for hiding this comment

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

Code looks good, tested with all browsers

@kmeleta
Copy link
Contributor Author

kmeleta commented Sep 16, 2021

@melissaperreault I discussed this with @Oliviammarcello but wanted to make sure you were aligned. We realized the defaults we were using for the new main blog settings were creating somewhat unnecessary visual breaking changes. I'm changing the layout setting to default to collage instead of grid since the collage layout is what dawn was released with. And for the image height we were defaulting to small but I feel medium (if not large) would be a slightly better match to the current image size. Let me know if you have any concerns or need clarification.

@melissaperreault
Copy link
Contributor

@melissaperreault I discussed this with @Oliviammarcello but wanted to make sure you were aligned. We realized the defaults we were using for the new main blog settings were creating somewhat unnecessary visual breaking changes. I'm changing the layout setting to default to collage instead of grid since the collage layout is what dawn was released with. And for the image height we were defaulting to small but I feel medium (if not large) would be a slightly better match to the current image size. Let me know if you have any concerns or need clarification.

Thanks for the ping!

  • Totally makes sense to default to Collage, aligned!
  • I would suggest to default to Medium, aligned!

ludoboludo
ludoboludo previously approved these changes Sep 17, 2021
Copy link
Contributor

@ludoboludo ludoboludo left a comment

Choose a reason for hiding this comment

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

Translations are in and only the default settings changed, seems good 👌

tauthomas01
tauthomas01 previously approved these changes Sep 17, 2021
Copy link
Contributor

@tauthomas01 tauthomas01 left a comment

Choose a reason for hiding this comment

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

LGTM 👍

ludoboludo
ludoboludo previously approved these changes Sep 20, 2021
@kmeleta kmeleta merged commit 8414d7b into main Sep 20, 2021
@kmeleta kmeleta deleted the blog-layout-updates branch September 20, 2021 19:23
phapsidesGT pushed a commit to Gravytrain-UK/gt-shopify-dawn-theme that referenced this pull request Sep 3, 2024
* Add new layout settings to main blog

* Update blog post image height settings

* Prevent full width single post in featured blog section

* Update featured blog limits and breakpoint cleanup

* Main blog collage fix

* Fix card text alignment

* Update tablet page width

* Change main blog defaults

* Update 13 translation files

* Update 7 translation files

* Update 1 translation file

Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com>
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.

Blog index - Layout improvements
8 participants