Skip to content
This repository has been archived by the owner on Aug 7, 2024. It is now read-only.

feat: add text on main page #1171

Closed

Conversation

santhoshbala0178
Copy link

@santhoshbala0178 santhoshbala0178 commented Mar 8, 2022

Fixes #1123

Changes proposed

  • Added "LinkFree" text along with Icon in Home Page and User Page
  • Added Loading state in Navbar.js to prevent items moving around
  • Added images folder under "src" to acces the logo

Check List (Check all the applicable boxes)

  • My code follows the code style of this project.
  • My change requires changes to the documentation.
  • I have updated the documentation accordingly.
  • All new and existing tests passed.
  • This PR does not contain plagiarized content.
  • The title of my pull request is a short description of the requested changes.

Screenshots

UserPage
HomePage

Note to reviewers

Copy link
Contributor

@github-actions github-actions bot left a 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.

@schmelto schmelto changed the title Feature-#1123 Add text on main page feat: add text on main page Mar 8, 2022
Copy link
Member

@EmmaDawsonDev EmmaDawsonDev left a 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

@santhoshbala0178
Copy link
Author

santhoshbala0178 commented Mar 9, 2022

@emmalearnscode, Thanks for the feedback. I have resolved all of the mentioned issues except one,

  • the image and text are not removed from the DOM when the screen gets smaller

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.?

@media screen and (max-width: 960px)
.p-menubar .p-menubar-root-list {
    position: absolute;
    display: none;
    padding: 0.25rem 0;
    background: #ffffff;
    border: 0 none;
    box-shadow: 0 2px 4px -1px rgb(0 0 0 / 20%), 0 4px 5px 0 rgb(0 0 0 / 14%), 0 1px 10px 0 rgb(0 0 0 / 12%);
    width: 100%;
}

src/Components/Home/Searchbar.js Outdated Show resolved Hide resolved
.p-menuitem-text {
font-weight: 600;
font-size: 20px;
}
Copy link
Member

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.

Copy link
Author

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={{
Copy link
Member

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.

Copy link
Author

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.

@EmmaDawsonDev
Copy link
Member

@emmalearnscode, Thanks for the feedback. I have resolved all of the mentioned issues except one,

  • the image and text are not removed from the DOM when the screen gets smaller

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.?

@media screen and (max-width: 960px)
.p-menubar .p-menubar-root-list {
    position: absolute;
    display: none;
    padding: 0.25rem 0;
    background: #ffffff;
    border: 0 none;
    box-shadow: 0 2px 4px -1px rgb(0 0 0 / 20%), 0 4px 5px 0 rgb(0 0 0 / 14%), 0 1px 10px 0 rgb(0 0 0 / 12%);
    width: 100%;
}

Don't worry about it just now. Unfortunately the prime react library isn't the greatest when it comes to accessibility.

@EmmaDawsonDev
Copy link
Member

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] Add text on main page
5 participants