-
Notifications
You must be signed in to change notification settings - Fork 0
Feat/91 layout hero #103
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/91 layout hero #103
Conversation
004c1fe
to
2fd0b7f
Compare
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.
Good PR! Code is simple and clean
font-weight: 600; | ||
font-size: 14px; | ||
text-align: center; | ||
line-height: 16px; |
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.
SHould this not be coming from the blocks? is it very different?
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.
Actually, it's not used. I missed removing these styles when i switched to the Blocks styling
<a | ||
href={button.link} | ||
class={buttonVariants({ | ||
variant: index === 0 ? "primary" : "tertiary", | ||
size: "sm", | ||
})} | ||
> | ||
{button.label} | ||
</a> |
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.
Should not be using a block button?
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.
The way that they look to be used in the UI is as links. Being semantically correct, should we not use anchors with button styling (for navigating) rather than buttons? I think doing would be inconsequential compared to doing it this way. let me know what you think
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.
What you said is actually correct, but the solution for that is different.
The "button component" as an "asChild" property, that will allow it to use the Child element: https://blocks.chain.link/?path=/docs/blocks-button--docs
So for example if I do
<BlockButton asChild><a></a></BlockButton>
It will not render the Button but just apply the style to the Anchor tag!
(this is a common practice also in shadCn)
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! That results in less code too so I will do it that way
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.
There's a bug with the component where it won't render when using asChild because of returning multiple react children. Tried to briefly troubleshoot, but even their example doesn't work
cae4eef
to
c7df3cd
Compare
IMPORTANT: Please do not create a Pull Request without creating an issue first.Any change needs to be discussed before proceeding.
Based off #90 and should be merged after it
Closing issues
closes #91
...
Description
Go to /ccip to see the hero (not mobile-friendly)

Changes