-
-
Notifications
You must be signed in to change notification settings - Fork 4k
feat: add text on main page #1171
feat: add text on main page #1171
Conversation
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.
It's great having you contribute to this project
Welcome to the community 🤓If you would like to continue contributing to open source and would like to do it with an awesome inclusive community, you should join our Discord chat and our GitHub Organisation - we help and encourage each other to contribute to open source little and often 🤓 . Any questions let us know.
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 for your contribution. Here's a few things I noticed and it would be great if you could work on them:
- the link doesn't work
- the icon is too small
- on small screens the menubar layout needs fixing
- the image and text are not removed from the DOM when the screen gets smaller
- The image/text is not centered on the profile page
@emmalearnscode, Thanks for the feedback. I have resolved all of the mentioned issues except one,
This is actually handled by the primereact library to hide the menu items below certain width, should I work on overriding all of the properties to show the text in smaller screens as well.?
|
.p-menuitem-text { | ||
font-weight: 600; | ||
font-size: 20px; | ||
} |
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 might look better if the icon and title disappeared on screens smaller than 864px, that's where the chips jump to 2 columns instead of 3.
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.
As mentioned in the comment below, this is handled by the primereact library to hide for screens smaller than 960px. We may need to override many properties to achieve this.
<Link | ||
to="/" | ||
aria-label="Go back to Home" | ||
style={{ |
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 should not be needed in order to center the icon and title. I think there may be some random margin on the github icon/version number that shouldn't be there which is making these things flex wrongly.
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 the margin is not added to the github icon and the content is centered only using justify-content: space-between, the icon and title are not centered to the page properly.
Don't worry about it just now. Unfortunately the prime react library isn't the greatest when it comes to accessibility. |
Thanks for your contribution. In the end we decided to take the home page in a different direction which has led to some merge conflicts in the pull request. If you want to have a go at fixing them then that would be great but we also understand if you would rather close this pull request in case it is too much work and you don't have the time. |
Fixes #1123
Changes proposed
Check List (Check all the applicable boxes)
Screenshots
Note to reviewers