-
Notifications
You must be signed in to change notification settings - Fork 5
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
Homepage: Add hero section for random featured site #145
Conversation
source/wp-content/themes/wporg-showcase-2022/patterns/_site-hero.php
Outdated
Show resolved
Hide resolved
source/wp-content/themes/wporg-showcase-2022/patterns/_site-hero.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, a few comments inline
Nice PR, lots to like here. I'd appreciate an eye by @fcoveram for a sanity check on the metrics, especially the max-width. here, 1920 from the mockups: In this case the dots actually scale a bit up compared to the 1320 view: At a glance, hard to test, the stroke in the mobile view looks a bit thick. It may be a trick of my eyes, and might not be something to fix here. But an option is to look at vw scaling strokes, like so: https://codepen.io/joen/pen/GRwXPop |
I fixed the code issues, and have pulled out the following issues from above (paraphrased). A lot is waiting on the spacing conversation.
This will depend on the new spacing values, I believe I grabbed the midsized-screen value to work with for now. On the meeting last week we decided that would work until the new spacing values were finalized. WordPress/wporg-parent-2021#105
Ah, I missed this on the design. I tried a version with clamp, it's mostly a guess to see if that would work, but it's roughly there. Maybe once this is pushed to staging a designer can take a look & use inspector to figure out the right values? Also to note, the spacing above the screenshot is a little awkward combined with the scaling dots, but when the spacing values are worked out this should scale too.
Currently this uses the same block as the single site meta list, so it was just inheriting the default spacing. I've updated the spacing when used without a label (like in the hero), but haven't address the scaling based on screen size (again, waiting on the spacing convo).
Okay, I've grabbed the Latest screenshots:
|
Nice! This is looking good. A few descrepancies I've noticed:
ThoughtsApologies for the late feedback, seeing this live has brought this out, but I was confused as to what the details in the "box" were especially because i don't know who/what "Conde Nast" is, haven't seen the meta detail box anywhere yet and the leading text, "Star-studded sites built with WordPress", seems to introduce the meta details but doesn't. Based on how it's written I would expect to see a list of sites. @marko-srb Did we consider adding labels next to fields for the homepage? Too verbose?
Also, would we consider dropping the meta details on mobile? Would that be a bad idea? The image is what pops, but instead I get a list of information that seems secondary and I am very unlikely to click on.
|
At a glance, looks good! I'd appreciate a sanity check by @fcoveram Probably nothing to block this PR, we can always follow up. |
@StevenDufresne @marko-srb For what it's worth, this is using the same block as the site details meta box, it just hides the labels. I could just untoggle that: |
Just a thing, There's no 'author' as metadata on the landing. The first row is actually just the name of the site in full. We made it through several pushbacks on the home having 3 metadata points:
Sorry if this wasn't clear enough... |
For the labels thing. I'd say we go without them on Home page. If we consider the name, url and tags, I am not sure we really need labels? Maybe it gets confusing sometimes, but I guess the name of the site is first what you see, and in most cases you see the same name on the site screenshot on the right (or initials) — at least that's the case for 6 sites I narrowed in showcase hero section for landing... If we go with a label though, it should say 'site name' not 'author', as we would avoid having less than those 3 data there on hero section... [ref is previous comment] |
Issue with that is, it can become a bit problematic in layout area, due to hero section attempt to have everything left aligned. But if you pursue this, please do test it, it may look better than I think and it is a quick test... |
Ah, okay. I made an incorrect assumption. In the future, could you annotate that sort of thing in the mockups (either in figma or when posting in github)? That would help.
I think that's fair, we can go without labels for now and if it seems unclear when merged, update it then 👍🏻 |
Nice, thanks for the update. In general, links in the metadata boxes are not underlined, and on the homepage they have a different link color: We can keep the external link icon as you have it for now, though there's a separate conversation on whether this should be the ↗ unicode icon instead, standardizing this on external links. CC: @fcoveram |
External link iconAgree with @jasmussen on replacing the external icon with the unicode symbol. Thumbnail on mobileI'm not sure what is the last version of hero section on mobile, but worth mention that in design the image is set to be full-width and reach the main container. See the red guidelines in the image below. Sorry the low contrast of the marks. |
Done ✅
I've updated this in the parent theme, so it should use the arrow now.
Okay, I've updated the spacing here too so it's edge-to-edge, but I'd like to merge this to get it onto the staging site so that you can see it action. |
Go for it! |
Fixes WordPress/wporg-main-2022#135. This adds a random site to the header as a "hero". The site is chosen from the "sticky posts" to avoid creating a second Featured category (the current category "Featured" is used for the homepage grid). The same approach was used in the first redesign iteration, see #37.
The Hero section displays a heading & blurb for Showcase, then the featured site's meta: author, domain, and category (should this be tags?).
The responsive styles are a work in progress, this will depend on how the spacing conversation goes I think. Currently the hero breaks into one column at 1179px, because that's when the image starts to look too squished. At 480px, the spacing and border are definitely too big, but that also depends on the outcome of the spacing issue.
To test
Due to the override we're using to fix this issue, if you don't have any sticky posts, it will show any random site.