-
Notifications
You must be signed in to change notification settings - Fork 494
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
Change inline code styling. #1203
base: main
Are you sure you want to change the base?
Conversation
5838e52
to
b19cbd1
Compare
Example pages to check:
|
styles/text.scss
Outdated
/* Improve `code` links. Don't use border, use underline instead. */ | ||
p > a:has(tt.docutils.literal) { | ||
@apply border-0; | ||
} | ||
|
||
/* Improve `code` links. Don't use border, use underline instead. */ | ||
p > a > tt.docutils.literal { | ||
@apply underline; |
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.
If we use underline, commands with underscores will look like this:
Also, the site uses border for links elsewhere, so if we do switch it, we have other styling to correct too. This is what the preview currently shows:
A link in the function description of st.logo:
An inline-code link in the class description of SQLConnection:
Links from the connections concept page (both standard and inline code):
The only links thatve been changed are inline code links within a parameter description, I 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.
I'd vote for leaving it as a border to be consistent across all links and just modify the color if that's what you want. Though I'd be a little confused by the color scheme if we make it a green underline for code (to always match the text color), then how do we handle the hover effect. We currently hover black -> red for the bottom border on links, but green -> red feels....seasonal. 😅
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 regular links we'll continue to use the existing design. What we're changing here is the design of links inside inline code.
So from your examples, only the SQL Connection example will be impacted by this PR. The rest will stay the same.
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.
In the connecting to data article, st.connection
is also inline code; it's just not inline code within Autofunction. Do you mean to exclude that as well?
Tips, Info, Warning: e.g. https://deploy-preview-1203--streamlit-docs.netlify.app/develop/concepts/multipage-apps/overview So we have inline code is green except:
Is that the correct intent? |
styles/text.scss
Outdated
/* Improve `code` links. Don't use border, use underline instead. */ | ||
a:has(tt:only-child), | ||
a:has(code:only-child) { | ||
@apply border-0; | ||
} | ||
|
||
/* Improve `code` links. Don't use border, use underline instead. */ | ||
a > tt:only-child, | ||
a > code:only-child { | ||
@apply underline; |
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 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 finds! Should be fixed now.
No, that should be allowed. The problem here seems to be that |
📚 Context
🧠 Description of Changes
Revised:
Insert screenshot of your updated UI/code here
Current:
Insert screenshot of existing UI/code here
💥 Impact
Size:
🌐 References
Contribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.