Added new logo suite component - #362
Conversation
🦋 Changeset detectedLatest commit: 5ff8655 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
🟢 No design token changes found |
|
|
Hi @rezrah This is amazing, thank you! One thing to bring up on the designs is the Universe preview URL and they have a placeholder “logo suite”. I notice this after I have handed off the designs and preview the Universe website on GitHub Pages, but it seems that between the heading and the logos they have a responsive spacing, 80px in big screens, 64px in mid screens and 32px in small ones. Logos are 48px tall in both the emphasis and muted versions. The current implementation favors the figma designs from Universe, while this responsive cohesive padding will favor what they have on GitHub pages, what are your thoughts on this? I think we may consider changing it to GitHub pages version to best suit this component for Universe? |
| import {Meta} from '@storybook/react' | ||
| import {LogoSuite} from './LogoSuite' | ||
|
|
||
| export default { |
There was a problem hiding this comment.
Can you please add a Playground story that works with controls? 🙌
jesskuo4
left a comment
jesskuo4
left a comment
There was a problem hiding this comment.
Some small comments / questions, I'll be sure to edit the index once some specs are finalized :)
|
|
||
| ```css | ||
| --brand-LogoSuite-color-logo-muted | ||
| --brand-LogoSuite-color-logo-emphasis |
There was a problem hiding this comment.
| --brand-LogoSuite-color-logo-emphasis | |
| --brand-LogoSuite-color-logo-emphasis |
Hi Reza! I was wondering if we could just use the brand tokens instead of creating logoSuite specific tokens, such as:
"value": "var(--brand-color-text-muted)",
"dark": "var(--brand-color-text-muted)"
instead of creating new tokens, that way it can be more centralized and reference the related colors. I agree with the logobar token though, as they are more specific to this component, what do you think?
There was a problem hiding this comment.
if we could just use the brand tokens instead of creating logoSuite specific tokens
Sorry, could you elaborate on this please? Specifically why we wouldn't want to make these tokens available?
For context on why these exist, it's to provide an abstraction on our text tokens, which I'm using for convenience but semantically aren't quite correct if used verbatim. Having dedicated tokens also provides an escape hatch for testing and/or customization, where you can change the value independently of changing a global token like text wholesale.
There was a problem hiding this comment.
I'll add my thoughts on this as well 😄
First, @rezrah it looks like you aren't actually referencing these tokens in the CSS. Is that intentional?
IMO it makes more sense to just use the colors directly as you have in the CSS. Its just less overhead to manage, and if we decide to add more variants in the future we can always add component specific tokens at that time (easier to add than to remove).
There was a problem hiding this comment.
Hi Reza! Thanks for mentioning this! I'm currently doing some work updating primer token names (specifically color) and some of the work with that includes simplifying or removing tokens that can potentially be replaced with some of our base tokens. Would love to hear your opinion on this 👍
There was a problem hiding this comment.
First, @rezrah it looks like you aren't actually referencing these tokens in the CSS. Is that intentional?
Oh wow 🤦. Thanks for pointing this out, must have forgotten to update those values. This is not intentional, no.
Totally appreciate the desire to reduce token footprint. In this instance I feel that providing these two color tokens is more consistent to how we approach API extensibility in Primer Brand generally (see Label for a similar example.
I'm currently doing some work updating primer token names (specifically color) and some of the work with that includes simplifying or removing tokens that can potentially be replaced with some of our base tokens.
👀 oh interesting. Is that work for brand, product libraries or both? In Primer Brand, we've historically created component tokens whenever we want to provide either escape hatches or additional low-level, user configurability. So it's quite a powerful tool, which our users also utilise regularly. Interested to hear more about this work if you can share some details? Also just FYI removing tokens usually requires managing breaking changes, so we'd need to plan ahead for things like that.
There was a problem hiding this comment.
Totally appreciate the desire to reduce token footprint. In this instance I feel that providing these two color tokens is more consistent to how we approach API extensibility in Primer Brand generally
Sure, don't consider this blocking feedback.
I'm currently doing some work updating primer token names (specifically color) and some of the work with that includes simplifying or removing tokens that can potentially be replaced with some of our base tokens.
@jesskuo4 is referring to the token name refactoring work that we're preparing for. So nothing new, just the same project that we've discussed in the past. In auditing the existing tokens we've found some areas that we can reduce the token footprint, but this is mostly scoped to control tokens so not necessarily a component-wide concern.
There was a problem hiding this comment.
Hi Reza! This storybook variant seems to show the align justified, but the header and description don't seem to be consistent with the default (eg: the one we see in logosuite align-start I was wondering if this needs to be updated?
There was a problem hiding this comment.
Yeah, thanks for bringing that up. I looked at other designs like Universe and noticed that their implementation of justified is only applicable to logos... the heading stays centralised. Happy to align-start the heading and description if you prefer though. AFAIK your design specs don't include a justified option, so I only added this in for sites like Universe.
There was a problem hiding this comment.
I see, sounds good! Thank you!
Would it be possible to have the text consistent with the other examples (eg: heading say heading and the description says body text....) I think align-start for heading and description for this example can help show that the justified option will only affect the logos 👍 I'll go ahead and also add this in interface guidelines as an option to have the logosuite logos justified, would that be ok with you?
| </> | ||
| ) | ||
|
|
||
| export const WithLinks = () => ( |
There was a problem hiding this comment.
Hi Reza! I just wanted to double check with this option of link and follow up on this slack thread, https://github.slack.com/archives/C04E44ZPNEA/p1689584732549729. What are your thoughts on this?
There was a problem hiding this comment.
Thanks for bringing this up 🙇. Great question. Ideally we wouldn't want to allow linking for the reasons I mentioned. However, I've noticed a conversation happening here, where they are thinking of using links in the logo area. I've pre-emptively ensuring that links work, which is why the story exists but I don't reference links in interface guidelines or react docs because it's not a recommended pattern. Does that make sense. Also happy to remove the story if you'd prefer.
There was a problem hiding this comment.
Hi Reza! Thanks so much, no worries at all, I think having a link option is great to future proof it and as long as usage is consistent/accessible, I don't think there will be a huge issue there. Since we are prioritizing for Universe, we can definitely work according to their current needs & add a link option :)
Thanks for sharing this thread, two things I want to bring up briefly:
Please arrange in hierarchy of sponsorship level. It should be Marquee largest and top row (3 max) // then Principal + Supporting next size (10-15 total) and second row // then Community smallest and lower row.
This was mentioned in the thread which is really interesting to me. In my head, I was thinking the marquee would be the bottom row as the community and least spotlight sponsors. And emphasis would be the largest top row. What is your opinion on that? Should we follow up in the thread to confirm if they are sure that the spotlight sponsors should be in an animation?
Also I think we can make the hierarchy less extreme for logo size. Marquee = 100%. Principal/Supporting = 85%. Community = 70%
This was also mentioned in the thread 🤔 regardless of what the top row is (marquee or emphasis), it seems that they might want it a bit smaller. What are your thoughts on this? If you agree, we can make the top row smaller as that seems like what they want. (I took the original measurements from the figma draft of universe)
There was a problem hiding this comment.
What is your opinion on that? Should we follow up in the thread to confirm if they are sure that the spotlight sponsors should be in an animation?
I agree we should respond to that thread. Before that, let's check in with @ajashams and @nsolerieu to confirm that our version is correct and aligns with their expectations for it? Perhaps they could also support the conversation around this as we try to discourage ad-hoc design changes that don't go through site design review.
There was a problem hiding this comment.
Sounds great, this is probably the final design thing, but since the designs choices here were originally referencing their original refactor work, I believe we just have to align with them on the current design-- potentially update/add detail the index interface guidelines a bit if usage expectations are unclear. I'll reach out to to Nicolas and have him check on this design as well 👍 After that, depending on his feedback, I can drop reply in the slack thread if needed, what do you think about that?
There was a problem hiding this comment.
if it's just interface guidelines, we can do that in a follow up PR. Is that okay?
|
Hi Reza! Thank you for engineering this so quickly, this is amazing ✨ One question I have about the divider is the length it spans and how we currently constrain the length. It seems like there are two options for us to consider,:
I was wondering if it's possible to test this component in the preview of the universe page? That may probably help with seeing the container of this component in an example? What do you think? 
|
jesskuo4
left a comment
jesskuo4
left a comment
There was a problem hiding this comment.
Editing the index file for reference images with msft-owned logos. Changed some small descriptions but please feel free to change accordingly or not 👍
Co-authored-by: Jessica Kuo <131988618+jesskuo4@users.noreply.github.com>
…into rezrah/add-logosuite
@jesskuo4 - there's a PR open for this now, with a preview url. I'll DM you the deets. |
jesskuo4
left a comment
jesskuo4
left a comment
There was a problem hiding this comment.
Changed some of the suggestions to be more clear/responded to comments, thank youu
There was a problem hiding this comment.
I see, sounds good! Thank you!
Would it be possible to have the text consistent with the other examples (eg: heading say heading and the description says body text....) I think align-start for heading and description for this example can help show that the justified option will only affect the logos 👍 I'll go ahead and also add this in interface guidelines as an option to have the logosuite logos justified, would that be ok with you?
jesskuo4
left a comment
jesskuo4
left a comment
There was a problem hiding this comment.
Looks good! I'll forward to Nicolas to check if usage / interface guidelines are aligned but everything LGTM 😄
josepmartins
left a comment
josepmartins
left a comment
There was a problem hiding this comment.
LGTM! 👍 left a minor suggestion
Co-authored-by: Josep Martins <josepmartins@github.com>
| </Do> | ||
| <Dont> | ||
| <img | ||
| src="https://github.com/primer/brand/assets/131988618/99c9fbad-5540-43b7-93b7-00ac5c22c868)f7" |
There was a problem hiding this comment.
This image seems to be broken (404).
There was a problem hiding this comment.
oof, thanks @danielguillan. The GitHub uploaded image URI included an ) which broke the link 😨. I can't be the only one stung by this 🤔, pretty dodgy bug.
Fixed though ✅
|
🚀 LGTM Relevant notes from site+brand review The visual weight of logos often differs a lot (ratio + fidelity) and with the current component setup everything is treated with a fixed height. This is not ideal when dealing with certain sets of logos. We should consider mentioning this in the guidelines and perhaps suggest a way to deal with logos featuring great fidelity and ratio differences. We often want to make sure the logo are evenly presented to be "visually" fair (which is important when thinking of sponsors or social proof sections). In past project we faced similar situation and never found a great way to deal with this, for example here on the customer stories page: Most of the example we've used in the design of the component (at least from what I can see in this thread) is ideal with logo that feature a very similar visual weight. Perhaps considering a |
|
Thanks @nsolerieu - @jesskuo4 perhaps something we can add to the interface guidelines in a follow up PR? |
|
@rezrah Yes, sounds good! I actually really wanted to include examples similar to what @nsolerieu mentioned but ultimately deleted that do/don't guideline because of the legal logo struggle (only being able to use msft-owned logo) 😭 That made it hard to replicate examples since on my first trial, it was hard to get similar examples as the home depot and american airlines that you mentioned with msft logos. Let me dig deeper and see how I can replicate something similar for the guidelines and follow with a PR. |
6 participants
Summary
New logo suite component for displaying vendor or product logos.
🔗 Interface guidelines
🔗 React documentation
🔗 Storybook examples
PR readiness
Please feel free to leave feedback and review this PR, even if all items aren't checked off below.
What should reviewers focus on?
Contributor checklist:
Reviewer checklist:
Screenshots:
Marquee effect
Screen.Recording.2023-07-31.at.11.00.26.mov
Stacked logos