-
Notifications
You must be signed in to change notification settings - Fork 8
[DOC-589] Button shortcode #259
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
Conversation
|
Deploy Preview Available Via |
site/content/3.11/_index.md
Outdated
|
|
||
| Normal button | ||
|
|
||
| {{< button text="Sign Up" color="green" position="right" >}} |
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.
text seems unnecessary/unused as we pass the label text in the body of the shortcode (.Inner).
What does position actually do?
|
@nikhil-varma can you please review the CSS changes to ensure that the styling of the buttons are aligned with the AG ones? |
| border: none; | ||
| cursor: pointer; | ||
| width: fit-content; | ||
| height: 2rem; |
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.
Let's use px instead of rem.. Ideally they should be used only for fonts .. But having px is much more consistent
| height: 2rem; | ||
| } | ||
|
|
||
| .button__wrapper__green { |
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.
Instead of such big names.. we can say button.primary and use it as button primary. Similar for others
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.
Well contextually.. can be button.green or button.red also. However, a more non-implicit naming convention like above is easier to understand and use it consistently.
| } | ||
|
|
||
| .button__link { | ||
| padding: 5px 1em 5px 1em; |
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.
Let's use px instead of mixed values.
| .deactivated { | ||
| pointer-events: none; | ||
| cursor: none; | ||
| border-bottom: 0; |
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.
Is this for 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.
This is for the button but will be removed in next commit
| } | ||
|
|
||
| .button__text { | ||
| border-bottom: 2px solid var(--green-600); |
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 this not be a part of button itself?
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.
Changing it to be part of button will display the border bottom with size of the entire button. By keeping as it is the border will be the size of the text
|
@nerpaula Done! |
|
@nikhil-varma re-done the shortcode and css from scratch. Much more clever now. Could you please review it now? |
|
@dandimeo Nice! That was my suggestion before as well. |
nerpaula
left a comment
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.
Thanks @dandimeo!
Description
Implement a {{< button link="" color="green/grey" >}}
to display a button
Example usage is shown in 3.11 homepage
3.11/_index.md content has to be reverted before merging
Upstream PRs