Updated home page and blog detail page ui #46
Conversation
…articles in feed option
…added hover styles
blocks/article-grid/article-grid.css
Outdated
| @@ -0,0 +1,126 @@ | |||
| /* use styles based on live milo libs */ | |||
| @import "https://main--milo--adobecom.hlx.live/libs/blocks/section-metadata/section-metadata.css"; | |||
| @import "https://main--milo--adobecom.hlx.live/libs/blocks/media/media.css"; | |||
There was a problem hiding this comment.
This is problematic, using .live URLS.
The better approach would be to use libs utils and load them via JS. We don't want .live resources loading for production content.
I would almost opt for most of this CSS living in https://github.com/adobecom/blog/blob/main/styles/styles.css if this grid should work on all pages. Keeping only direct grid related css here. Media, banner, section-metadata, etc living in /styles/styles.css
There was a problem hiding this comment.
Thanks for the flag. Followed Momiko's advice on using loadStyle instead of @import.
Let me know if that needs to be updated further, thank you
| .article-grid .media .text h2 { | ||
| display: -webkit-box; | ||
| -webkit-line-clamp: 2; | ||
| -webkit-box-orient: vertical; |
There was a problem hiding this comment.
Are the vendor prefixes needed?
There was a problem hiding this comment.
https://developer.mozilla.org/en-US/docs/Web/CSS/-webkit-line-clamp
The -webkit property is needed for the line clamp function to work, let me know if this needs to be updated further
There was a problem hiding this comment.
Not blocking this and don't have all the answers, but I wonder a bit seeing at the amount of code added to scripts.js and utils.js which to some degree seem to be an existing issue.
Usually blocks should be contained and not bleed into the general project - if we look at how bacom does it https://github.com/adobecom/bacom/blob/main/scripts/scripts.js their scripts.js is ~70 lines if you discard the locale declarations.
Also it seems that you need to re-use quite a few methods like decorating placeholders yourself, which makes me wonder - why isn't milo handling this?
Placeholders should be decorated from milo, rather than you having to manually call the method.
I don't have enough context/project knowledge to approve or request changes, but some food for thought
blocks/article-grid/article-grid.css
Outdated
| @@ -0,0 +1,126 @@ | |||
| /* use styles based on live milo libs */ | |||
| @import "https://main--milo--adobecom.hlx.live/libs/blocks/section-metadata/section-metadata.css"; | |||
There was a problem hiding this comment.
You should be able to use the loadstyle method https://github.com/search?q=repo%3Aadobecom%2Fblog%20loadstyle&type=code or as @rgclayton suggested, have this live in a more general file.
There was a problem hiding this comment.
Thank you so much! Used loadStyle instead of @import, thankss
| @@ -27,7 +25,7 @@ export default async function init(block) { | |||
| const bannerImage = document.createElement('div'); | |||
| const bannerText = document.createElement('div'); | |||
| bannerImage.classList.add('banner-image'); | |||
| bannerText.classList.add('banner-text'); | |||
| bannerText.classList.add('banner-text', 'dark'); | |||
There was a problem hiding this comment.
Is the .dark option something that should be author-able or is this a new default? With out too much context on existing vs new, I just wonder if this will have any regression impact.
There was a problem hiding this comment.
I don't think we should bake this in. As Ryan P mentioned it should either use the default or be authorable. We do not want to break existing pages using the banner.
There was a problem hiding this comment.
Thanks for the feedback, added in variant for light, dark, no-overlay for more customization options
Staging link for banner block here: https://redesign-article--blog--adobecom.hlx.page/en/drafts/snowie-testing/blocks/banner
There was a problem hiding this comment.
Overall this looks good visually on the after pages but as others have mentioned there is a lil more consideration I think as to when/where we load these additions.
I think this would be better as 2 PR's (article pages, and blog HP) just a thought, but not totally necessary as your probably trying to release this as a unit. Id have to do some testing and look at the latest implements, but we might be able to leverage metadata 'templates' etc to add these files and then pages could adopt them w/ more control. (I also know that also can mean library templates which is different grouped content, so i'd want to vet a gut check before totally suggesting this route ;) )
I plan on joining the upcoming meeting to help iron some of this stuff out and get a better handle on /blog setup as I'm a little out of the loop on this specific repo so I don't want to suggest too much w/ out giving it a lil due diligence myself. Thanks
|
Will close this PR and separate it into 2 PRs for update in home & article detail page |
|
Closing this PR as created the following 2 PRs for the redesign updates: Will mark those as |
Updates:
home pageuimasonryandbrickblockmediablockarticle-gridblock and updatedbannerblock forfeatured blogandlatest articlessectionblog detail pageuiarticle hero bannerandarticle-metauisidebarto blog contentArticle Test URLs:
Homepage Test URLs:
Figma of updated design:
The homepage content on staging is provided by Kristine so it should be good to go live. But wondering if
Let me know if any code adjustments is needed as I've applied code changes only within this repo (no
Miloupdate).Thank you.