-
-
Notifications
You must be signed in to change notification settings - Fork 46
Update screenshots to new UI #1560
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
base: main
Are you sure you want to change the base?
Conversation
Code Coverage Summary
Diff against main
Results for commit: c184349 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
Unit Tests Summary 1 files 26 suites 1m 58s ⏱️ Results for commit c184349. ♻️ This comment has been updated with latest results. |
Unit Test Performance Difference
Additional test case details
Results for commit c786d6a ♻️ This comment has been updated with latest results. |
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.
Great work in breaking down the Rmd
and all the bs-*
files seem nice 💯
I think the teal-app-components-***.svg
files can be improved
|
||
Please install `bslib` package before you run the code below. | ||
|
||
### `teal.bs_theme` `R` option | ||
|
||
``` | ||
``` |
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 extra space required?
Both here and in the other code chunks
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.
Without adding too much detail, I think we could either change "Header" to "Header & navigation"
Or add a new horizontal rectangle below "Header" with "Navigation & global Actions" (or use a better naming)
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.
Header font is different (old one was using "Helvetica", new seems to use a Serif font)
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 outer border is cut and doesn't fill the whole image and when hovering this artifact is noticeable.. Low priority, but noticeable
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.
At this moment I think it would be better to insert an embedded screenshot of the teal application instead of using SVG elements in the hover
image.
If you still prefer using "pure" vectorial image, it looks different on the browser because you are saving the SVG with the font reference, it is not embedded.
To avoid this there are a couple of alternatives:
- You need to select the text and convert to path (but this defeats the purpose of using svg)
- Encode it in base64 as this link says
- Use a commonly available font
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 outer border is cut and doesn't fill the whole image and when hovering this artifact is noticeable.. Low priority, but noticeable edit: wrong file
Pull Request
Fixes #1544
This updates the screenshots of the shiny apps shown on the bootstrap vignette.
The svg on teal/vignettes/images/teal-app-components-hover.svg used on the "Getting started with teal" are updated. In Inkscape they look good but on my browser the example doesn't:
I also deleted some images I couldn't find where were they shown (Not on blueprints or other).
I run
devtools::build_article()
to verify that the vignette is updated and looks good.