-
-
Notifications
You must be signed in to change notification settings - Fork 643
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
feat: new styles implemented #534
Conversation
Styles to match new brand have been added in tailwind config. In the process of making sure all typography and colors match new brand.
✔️ Deploy Preview for asyncapi-website ready! 🔨 Explore the source changes: b3dc206 🔍 Inspect the deploy log: https://app.netlify.com/sites/asyncapi-website/deploys/6225f5c6f344b60007a77d9f 😎 Browse the preview: https://deploy-preview-534--asyncapi-website.netlify.app |
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.
I know that it's a draft PR but I see some problems.
I checked live preview and it's amazing 🚀 I am not sure but everything looks almost finished 😅 In the meantime, are you also working on a page for style guides (I ask because of the PR description)?
@magicmatatjahu I have raised an issue for this so someone can help me set up the template. I tried doing it myself but I couldn't get it to work right. Here is the issue: #532 I think @yashsehgal is wanting to help out with it! |
If you need help I can show you how to setup new page on video call :) When do you have time? Of course if @yashsehgal don't have problem with second guy with help 😅 |
@magicmatatjahu yes! I would love that. I think @yashsehgal mentioned he was having issues with it on the issue I raised? Perhaps you can help us both 😄 |
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.
Three more comments :)
components/typography/Heading.js
Outdated
let classNames | ||
const Tag = `${level}`; | ||
|
||
if (typeStyle === 'heading-xl') { |
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.
Can I have question? What is a difference between heading
and body
? I mean e.g. heading-lg
and body-lg
?
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.
Sure, heading
is for heading type treatments and in most cases have a heavier weight and darker color than the paragraphs, which are called body
to represent body copy. Here is a visual breakdown of these styles and the class names assigned to each.
The names in the bubble
are the identifiers which will be passed into typeStyle
within each component and the classnames below that identifier are the tailwind classnames that are assigned to each identifier.
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.
Ok, thanks! :) However, can we use here switch cases
syntax not if else
?
Hi @magicmatatjahu 👋 I think this is about ready for review. There might be some small things that I might have missed but IMO I don't think we should wait much longer to merge these changes, for things like that we can just open separate PRs. Initially, I thought it would be a good idea to merge this PR at the same time as #539 (brand guidelines pages), but honestly after thinking about it I don't see why the making of the brand guidelines pages have to hold up the release of this PR, but wondering what others think. |
Hey @mcturco sorry for intervening in the PR 😅, as I am not much related to the PR and issue. Here are some of the changes I think you should need to consider:
|
@magicmatatjahu Thank you for the tips! Check now and see if that's better 😄 |
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.
Two comments about switch case
:)
@mcturco 👌🏼 Lets see what other people will think :) Could you write about that PR on Slack as we discussed? |
Hey @mcturco, I saw the preview right now. Really, great work done by you in typography, styling, and design. You nailed it ✨. Overall, PR looks top-notch and fully complete. Just look like a few finishing touches are needed and then, this PR will boom the website 💥. Here are some suggestions from my side:
Yep, I completely understand that you have made a default styling for all the links on the website, but I think we have to make the hover effect of this link the same as that of
This error is still on the website right now as well. Looks like there is a very fine gap between the top and navbar of the website. I don't know why it is there and how to resolve it. I will just suggest, if you find this error, kindly resolve it 😄.
I'm not sure why you put extra spacing on the left of the title. Is it made intentionally? From my point of view, it should be left-aligned as it is right now on the website. |
Hey @akshatnema, thank you so much for your kind words and your review ❤️
Good find! I have updated the code with your suggestion 😄
I don't see this on my end, even when checking in other browsers (chrome, firefox, safari). Either way, because this is a current problem on the website, we should probably raise a separate issue and debug it there.
Thank you for finding this bug! That definitely wasn't intentional... not sure how that happened, must have been while merging changes. Just fixed that as well! |
Seriously, Missy needs more feedback. it's a big change and we don't want to merge with two reviews. cc @derberg @boyney123 @fmvilas @alequetzalli |
The roadmap page isn't still visible? Any idea why? |
It is never visible because someone could make a console log of our credentials (I guess, I haven't configured repo config myself). |
Oh yeah, you're right. Cool, then I'll blindly say "LGTM 👍" 😄 |
Scanned through the 45 files of changes, couldn't see any obvious problems, also spent time looking at the preview (which looks amazing btw), can't find any problems either... so from what I can tell it's looking good! |
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.
LGTM!
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.
🚢 🚢 🚢
/rtm |
Currently working on a new version of the website with the new branding applied across all elements. Styles to match new brand have been added in tailwind config. In the process of making sure all typography and colors match new brand. Some small edits to spacing are being made here as well.
Also - wanted to make it known that there are going to be little changes in several files. If it makes more sense to separate these into several PRs, let me know! Just wanted to post the progress so far.
cc: @magicmatatjahu
UPDATE:
New styles have been implemented based on the current Design System styles 🎉
Here is a screenshot preview, but you may also view the changes via the deploy preview link below:
Some things that were edited:
Heading
component (applies tailwind classes based ontypeStyle
prop, with ability to change the heading level)Paragraph
component (applies tailwind classes based ontypeStyle
prop)TextLink
component (applies tailwind classes by default for inline links)Do leave your feedback in this PR if you have it! 😄