-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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
Changes to make the docs homepage cards look consistent #43166
Changes to make the docs homepage cards look consistent #43166
Conversation
✅ Pull request preview available for checkingBuilt without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
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 as it's a replica of #43164 (scss commit), 👍
/area web-development |
For finishing touch, few things that are coming to my mind:-
|
@aj11anuj, I appreciate the time taken for the review :) So regarding the 'Documentation word wrap' which you addressed in the second pointer, We need not change the front matter for that as CSS should adapt to html and not the other way around. I'll just reduce the padding which will render the text properly on most viewports I'm afraid that the first and the third issues can't be addressed with this PR, As they need some changes over the homepage index . I'd be happy to see a follow-up PR from your end to propose these changes |
ee226f3
to
6957ce8
Compare
Ideal outcome: also use flexbox and a variable layout so that there are fewer columns - maybe just 1 - on a narrow, mobile-device viewport. |
I recommend against making content change in this PR. Style changes should work for the content we have, which is in 13(ish) localizations, and ideally for the content we might have in the future. Pages can get reworded etc. |
I get your point @sftim, I've removed the background and focused more on the alignments in the recent push. The page would work just fine on most viewports now |
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.
Though that's not a standard viewport, I do see the problem here @sftim. I'll figure something out
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 you need / want to change the HTML templating to help this land, that's fine.
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 think it's more of a css ball game. Should we perhaps try borders if backgrounds don't work? You see the problem here is actually addressing the disconnect between the text alignment and the centred 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.
/hold I'd like to try out some ideas |
/unhold |
Reduced padding to make text compatible Some amendments ; bg removed Buttons left aligned Grid is more suitable for context added a wrapper div with flex
f2eec2b
to
0b4c196
Compare
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
I'd like to hear what other reviewers 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.
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.
Ah, I understand that It might look a little off sometimes 😬. While we've tried some other approaches, I personally feel that this one offers the best consistency and alignment over many viewports
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. @sftim - on training https://deploy-preview-43166--kubernetes-io-main-staging.netlify.app/training/. - There was light background for tile with no border, do you think is it worth to get inline with that.
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 light background for tile with no border, do you think is it worth to get inline with that.
I don't - the docs have a simpler look whereas the training page is a bit more promotional.
But, thanks for suggesting 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.
I'm going to treat #43166 (comment) as LGTM
/lgtm
from that.
/approve
from me
and thank you for the PR!
|
||
button { | ||
height: min-content; | ||
width: auto; |
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.
A bit of CSS calc()
here could improve the look further. If you want to put the work in there (it's fine not to!)
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.
Sure, Might as well try that. If I'll like what I see, A PR will be on it's way
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aj11anuj, sftim The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
LGTM label has been added. Git tree hash: 257156edd54fdb7ec524340f41d4992b72369ca9
|
Closes #23914
This PR is an extension of #43164 to cover the styling changes and make some additional markup changes
Please have a look at the Deploy Preview for a better perspective of the PR