-
Notifications
You must be signed in to change notification settings - Fork 144
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
[Bug] Fix text alignment and enable href in Button #881
Conversation
for more information, see https://pre-commit.ci
View the example dashboards of the current commit live on PyCafe ☕ 🚀Updated on: 2024-11-19 12:25:19 UTC Link: vizro-core/examples/dev/ Link: vizro-core/examples/scratch_dev |
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.
Looks good! I had noticed the bad link alignment on buttons when I added one to ViViVo. What is the unminified change to CSS to fix it? Oh, I see you linked to it 👍 Can my custom CSS on https://github.com/mckinsey/vizro/pull/824/files/a304db5e8797bdb53e6fcb53f9cd78fa3d94e3fb#r1815769534 Judging by the pycafe preview I think the
lines 31-35 here be removed now?line-height: unset
can be removed but the margin-bottom: 12px
is still needed?
The Button.href
is probably worth a brief mention explicitly in the docs and crosslink/mention between that Card.href
.
vizro-core/changelog.d/20241118_113651_huong_li_nguyen_button_alignment.md
Outdated
Show resolved
Hide resolved
Ahh, I didn't realise you had added custom CSS. All of this can be removed now - I'll quickly push that change 👍 |
I think the |
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 really looks good.
Thanks for the https://py.cafe/huong-li-nguyen/vizro-button-link-examples that shows how relative and absolute href paths work properly!
Will wait for @stichbury approval on the docs changes before merge |
@@ -482,9 +482,11 @@ img[src*="#my-image"] { | |||
|
|||
### Create a navigation card |
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.
For review for @stichbury
Ahh, i see - it's not related to the the button text |
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 🌟 with just a few minor suggestions for clarity.
for more information, see https://pre-commit.ci
Description
I noticed that the text inside the button was misaligned in some of our demos and our 404 page. This was the case for all buttons that were used as anchor tags, so where
href
was used.This has been fixed in the vizro-bootstrap repository here: https://github.com/McK-Private/vizro-bootstrap/pull/36
While fixing this I was actually wondering why we did not enable the
href
argument for theButton
. For me, it functions similarly as ourCard
, where we have also enabledtext
andhref
. This also seems to be a common use-case, so in this PR I would like to add the argument in unless we had good reasons to not do so?Screenshot
Before
After
Notice
I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":