-
-
Notifications
You must be signed in to change notification settings - Fork 62
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: Version support page #600
Conversation
✅ Deploy Preview for zh-hans-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for es-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for de-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for ja-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for pt-br-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for hi-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for new-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for fr-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
<dd> | ||
{% set version_date = stats.lastCommitDate | shortDateFromISO %} | ||
{% set dateline = site.homepage.versions.dateline | safe | replace("VERSION", "<a class=\"text-dark\">HEAD</a>") | replace("DATE", version_date) %} | ||
<div class="eslint-versions-container"> |
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.
Two small notes about the layout after this change, I'm not sure how important they are and whether they're intentional:
- The ad and the versions are moved up a bit (when compared to the current eslint.org homepage).
- On a mobile screen, the Version Support link seems too close to the horizontal line below it.
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 1, best I can tell, adding the "Version Support" link pushed everything up, but I have no idea why. Someone's who done web development more recently than I can probably figure it out pretty quickly. 😄
For 2, I've updated the styles.
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.
- Everything moved up a little bit because of
padding-block-end: 2rem; padding-bottom: 2rem;
This padding seems to force content to move upwards
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 1, the ad and version shift slightly upward due to the align-items: center
property applied to both the .hero>.content-container and .hero .grid elements. The align-items: center
property attempts to center-align the child elements, so adding a new element to the right column causes it to shift up slightly.
To fix this, we can do the below steps:
step 1: align .hero>.content-container
element normally
step 2: The left grid span-1-7 content-container
element has some padding applied. Apply the same padding to the right grid as well.
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!
Co-authored-by: Milos Djermanovic <[email protected]>
Co-authored-by: Milos Djermanovic <[email protected]>
Co-authored-by: Milos Djermanovic <[email protected]>
Co-authored-by: Milos Djermanovic <[email protected]>
Co-authored-by: Milos Djermanovic <[email protected]>
Co-authored-by: Milos Djermanovic <[email protected]>
The alignment isn't exactly what it was, but I think it's close enough without getting too hacky. |
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, thanks! Leaving open for others to verify and for @nzakas to merge when appropriate.
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 layout appears to be breaking in mobile view.
Steps to reproduce:
Open the preview link on a mobile device, scroll the table to view the data in the rightmost column, and then scroll back to the top.
Screen.Recording.2024-07-12.at.9.33.05.PM.mov
Can we use the Eleventy transform to wrap the table inside a div and make it scrollable on mobile devices to prevent layout issues?
If everyone agrees, I can work on this in a separate PR. This way, we'll have a solution ready for future instances where we need to display tables with many columns.
@amareshsm is that possible without forcing us to change the way we write tables in markdown? If so, that sounds like a good idea. |
Converting to Draft because I'm unsure of the timing around releasing this page. Will talk with the HeroDevs folks and then decide. |
I was able to use CSS only to create a horizontal scrollbar for the table on small screens. |
@@ -208,6 +212,11 @@ table { | |||
background-color: var(--lightest-background-color); | |||
padding: 0.25rem 0.5rem; | |||
} | |||
|
|||
@media screen and (max-width: 768px) { | |||
display: block; |
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.
display: block; | |
display: block; |
I came across this approach as well. Using display: block for a <table>
tag will change its display behaviour. When we set display: block, the <table>
element will behave like a block-level element instead of a table. I'm not sure if this is the best approach, so I'll check for any potential side effects.
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.
Thanks. I think we can fix that pretty easily using CSS.
Co-authored-by: Amaresh S M <[email protected]>
Prerequisites checklist
What is the purpose of this pull request?
What changes did you make? (Give an overview)
/version-support
/version-support
Related Issues
Refs #594
Refs eslint/eslint#18621
Is there anything you'd like reviewers to focus on?