search

open-sauced / hot

🍕The site that recommends the hottest projects on GitHub.
https://hot.opensauced.pizza
MIT License
426 stars 148 forks source link

feat(primarynav): adding AIOutlineStar icon and Star us on Github text to PrimaryNav #293

Closed AlexVCS closed 2 years ago

AlexVCS commented 2 years ago

What type of PR is this? (check all applicable)

Description

This PR is working to add the icon and star us on GitHub text to the nav

Related Tickets & Documents

Feature #289

Mobile & Desktop Screenshots/Recordings

Screen Shot 2022-08-05 at 1 41 43 PM

Added tests?

Added to documentation?

[optional] Are there any post-deployment tasks we need to perform?

[optional] What gif best describes this PR or how it makes you feel?

netlify[bot] commented 2 years ago

Deploy Preview for hot-sauced-ui ready!

Name Link
Latest commit 0f43cfcd8b9b9e63b3c651e3bc05f50c000dfe37
Latest deploy log https://app.netlify.com/sites/hot-sauced-ui/deploys/62fbb1836c1e3200085c9f56
Deploy Preview https://deploy-preview-293--hot-sauced-ui.netlify.app/
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

0-vortex commented 2 years ago

npm run format is your friend! Should fix most of the red dots 🍕

bdougie commented 2 years ago

checking in @AlexVCS to see if you have everything you need.

AlexVCS commented 2 years ago

checking in @AlexVCS to see if you have everything you need.

Screen Shot 2022-08-08 at 8 22 56 PM

Hey Brian, as you can see in the screenshot, it looks decent on desktop. Needs refinement for mobile. A family funeral has come up all of a sudden, so I hope I can get feedback on what I've got and work more on this Thursday or Friday (my apologies for the delay).

bdougie commented 2 years ago

No worries @AlexVCS. A few things.

Screen Shot 2022-08-10 at 9 40 15 AM

AlexVCS commented 2 years ago

Thanks for the feedback Brian! I've applied what you said, let me know if I need to make any changes!

Screen Shot 2022-08-12 at 1 02 46 PM
bdougie commented 2 years ago

Mobile view for sign button is off image

AlexVCS commented 2 years ago

Gotcha, should be fixed now!

Screen Shot 2022-08-12 at 3 02 20 PM
bdougie commented 2 years ago

Just realized this does not show when logged in, that would be ideal.

AlexVCS commented 2 years ago

Not the most DRY, but I have it showing up when logged in now too. I'm using another tailwind anti-pattern, I was looking up a few things about how to avoid this, but am unsure and I know that issue #267 is still being worked on. If you've got any feedback on how to get away from that now, or at all please let me know!

Screen Shot 2022-08-14 at 12 20 47 PM
bdougie commented 2 years ago

Only because I saw you are looking for work on your Twitter, I will need to push back once more and request that this div be placed into a new reusable component. You can put it in the same file, but duplicating code is not going to impress hiring managers. 

const StarTheRepo = (): JSX.Element => (
   <div className="hidden md:flex items-center text-osGrey">
     <a
        href="https://github.com/open-sauced/hot"
        rel="noreferrer"
        target="_blank"
      >
        <AiOutlineStar className="inline-block mr-[10px]" />

         <span className="text-sm mr-[10px]">Star us on GitHub</span>
     </a>
  </div>
)
AlexVCS commented 2 years ago

Thanks Brian! I made it a reusable component.

Screen Shot 2022-08-15 at 7 01 50 AM
0-vortex commented 2 years ago

hidden makes it hidden on all screen while md:flex makes it flex visible on medium devices and up, we should apply a mobile-first approach to this if possible, rather than duplicate the element in 2 places for example the header (hidden md:flex) and footer (flex md:hidden) - this is an anti-pattern.

I propose:

    <div className="flex md:hidden w-[24px] h-[24px]">
      <GiHamburgerMenu size={24} />
    </div>
  </div>
</Menu.Button>

Doing all the above should provide for a seamless experience on all platforms.

Sorry for the checkbox style changelist, when moving markup like child into parent containers, it makes proposing changes very verbose and I wanted for these to also make sense if you encounter similar problems in the future! 🍕

AlexVCS commented 2 years ago

hidden makes it hidden on all screen while md:flex makes it flex visible on medium devices and up, we should apply a mobile-first approach to this if possible, rather than duplicate the element in 2 places for example the header (hidden md:flex) and footer (flex md:hidden) - this is an anti-pattern.

I propose:

  • removing hidden and md:hidden classes in favour of simply flex while changing the font to font-Inter, the final class could look like this: flex items-center text-osGrey font-Inter
  • changing line 21 from <span className="text-sm mr-[10px]">Star us on GitHub</span> to <span className="text-md font-light mr-[10px]">Star us on GitHub</span>
  • move the closing div on line 61 to line 65 to encompass the GiHamburgerMenu and its parent
  • making the mobile hamburger menu slightly bigger to be in line with the star link and opensauced logo, increasing the sizes from 20px to 24px, with the line 61 change this element should look like
    <div className="flex md:hidden w-[24px] h-[24px]">
      <GiHamburgerMenu size={24} />
    </div>
  </div>
</Menu.Button>

Doing all the above should provide for a seamless experience on all platforms.

Sorry for the checkbox style changelist, when moving markup like child into parent containers, it makes proposing changes very verbose and I wanted for these to also make sense if you encounter similar problems in the future! 🍕

The button and icon appear on mobile with these changes. Brian previously mentioned hiding it for mobile so that's why I had them hidden on mobile. The spacing and alignment is OK when signed in, but not great when not signed in. I can work to optimize the spacing for mobile, is that what you'd like? I can make the sign in button bigger, and decrease the size of the star us on GitHub text.

I've included screenshots after making the changes you recommended in mobile view.

Signed in:

Screen Shot 2022-08-15 at 9 59 09 AM

Not signed in:

Screen Shot 2022-08-15 at 9 59 56 AM
0-vortex commented 2 years ago

You are right, my mobile screen is too big 😓

AlexVCS commented 2 years ago

I pushed the changes up. I feel it would be better to remove the icon and text on mobile, if any solution seems better to you please let me know.

bdougie commented 2 years ago

Yeah, mobile doesn't work with all the words. An alternative would be only showing the star. Another option is using a smaller font size + less words

Up to you and your creativity, but 'hidden sm:flex' works as a bare minimum

update:

I looked at the options and hiding this on mobile is the best case, without getting design help. Event with the start alone if looks off.

Screen Shot 2022-08-16 at 7 14 12 AM
AlexVCS commented 2 years ago

Yeah, I've gone back to making it hidden on mobile and pushed up the change.

Screen Shot 2022-08-16 at 11 03 27 AM
github-actions[bot] commented 2 years ago

:tada: This PR is included in version 2.24.0-beta.1 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

github-actions[bot] commented 2 years ago

:tada: This PR is included in version 2.24.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: