-
-
Notifications
You must be signed in to change notification settings - Fork 235
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
fix: improve layout design for large screens #1231 #1437
Conversation
β Deploy Preview for design-insights ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
β Deploy Preview for oss-insights ready!Built 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.
PR Compliance Checks Passed!
Screen cap before and after please |
@bdougie, done. Thank you but it is yet to pass the checks |
Hey @RitaDee, Thanks for taking this on π . A few notes after reviewThe preview shows that the Screen.Recording.2023-07-27.at.07.01.53.movin line with setting the width in lg to display the button, we should as well hide the |
Closing this as the solution seems a little scope crept to me ππ« ! |
@RitaDee if you want to keep working on this and get some help, I would be more than happy to walk through with you in this PR. It might be worth taking a look at these codes if you are still interested. |
Hello @a0m0rajab, I will be glad to receive the walkthrough. Kindly let me know what time works best for you. Thank you |
Awesome, to set expectations, this is a bit of a tricky task (you might need to change around 6-7 files till you get it fixed), there is a lot of complexity in it, and that's okay to find it hard, yet it's a bit of a great way to get familiar with the code. To implement this, you can do the next:
Each iteration will teach you about the code, the logic, and how it works. As for this task, Get IdentifierHere is how you can do this: www_screencapture_com_2023-7-31_03_05.mp4From this at the end, you can see that I got; I will go to my code editor right now or GitHub to find these classes. I am using VsCode so here is how to do it with it: www_screencapture_com_2023-7-31_03_14.mp4Here is how to do it with GitHub:
www_screencapture_com_2023-7-31_03_13.mp4GitHub Result: Understand the codeSince we got the related code right now, we will need to understand it. The first thing to notice is that it has a comment saying it's a column and it's using tailwind: {/* Column Last 30 Days */}
<div className={clsx("flex-1 lg:min-w-[150px] hidden lg:flex justify-center")}>
{last30days ? <Sparkline data={last30days} width="100%" height={54} /> : "-"}
</div> Tailwind
If you need to become more familiar with it, it's just a short style of writing CSS as a class. It has a special identifier as well. For example:
The previous code is converting to: if small screen, then the width should be 50% if large screens, then the width is 100% We can see that the keyword hidden is being used as well. Here we can have three questions:
Make changesSince we get that, we can now change the code and see the results. We can change the The resultAfter doing changes in code, you need to run it on your local machine to test it, get the screen, and see if you need to fix/change anything else. RepeatYou can repeat the same steps for a 4-5 times till you get the full expected result, again this is a bit tricky task though. If you need any help or if you feel stuck and like to pair program then I would be happy to do that on discord: contributors channel. |
Thank you, @a0m0rajab I will check it out. |
I just had a 30-40 minutes call with @RitaDee to apply the d1d76f1 commit. Thank you for your time! As for my opinion, I think that this PR is good right now but, I have two things in consideration:
|
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.
@RitaDee Thank you for your effort and force pushing the PR,
LGTM! π
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 for this fix @RitaDee. Welcome to the community π
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 PR is included in version 1.58.0-beta.3 π The release is available on GitHub release Your semantic-release bot π¦π |
## [1.58.0](v1.57.0...v1.58.0) (2023-08-02) ### π§βπ» Code Refactoring * merge `authSession` function into `useSession` hook ([#1391](#1391)) ([d6c230d](d6c230d)) ### π¨ Styles * add width and max width for created at in contributor-profile-tab ([#1429](#1429)) ([5614886](5614886)) * remove irrelevant padding in page divider ([#1427](#1427)) ([5faaa4e](5faaa4e)) ### π Features * add devcard button to user profile ([#1339](#1339)) ([6a1dbdc](6a1dbdc)) * new contributor highlight card ([#1443](#1443)) ([c88000b](c88000b)) * update redirect to feed page for unauthenticated users ([#1464](#1464)) ([6d8505f](6d8505f)) ### π Bug Fixes * add checkbox id name if not available based on label ([#1466](#1466)) ([68f66a7](68f66a7)) * add navigation to improve accessibility ([#1436](#1436)) ([d1d85f7](d1d85f7)) * contributor profile tab click state flicker ([#1432](#1432)) ([c9cf8ed](c9cf8ed)) * Deleted page button changed to delete highlight ([#1419](#1419)) ([d502605](d502605)) * fixed bug on chatbot button overlay ([#1420](#1420)) ([2a94583](2a94583)) * hide onboarding button on mobile ([#1460](#1460)) ([f63f240](f63f240)) * Improve Keyboard Accessibility for Notification Icon ([#1435](#1435)) ([05291c0](05291c0)) * improve layout design for large screens [#1231](#1231) ([#1437](#1437)) ([d8ae808](d8ae808)) * misaligned chat button close icon ([#1422](#1422)) ([60f22bd](60f22bd)) * mismatched selection color ([#1430](#1430)) ([8a1d37b](8a1d37b)) * reduce tab font size in contributors profile page ([#1413](#1413)) ([238dc2f](238dc2f)) * show repo filters on initial `feeds` route visit ([#1426](#1426)) ([833ee30](833ee30)) * tab inconsistency in user profile matching the url ([#1403](#1403)) ([f8c6766](f8c6766))
π This PR is included in version 1.58.0 π The release is available on GitHub release Your semantic-release bot π¦π |
Description
This PR fixes the issue of Odd design for large screens #1231
What type of PR is this? (check all applicable)
Related Tickets & Documents
Desktop Screenshots:
Before:
After:
Added tests?
Added to documentation?