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

[AlexVCS]

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

• [x] 🍕 Feature
• [ ] 🐛 Bug Fix
• [ ] 📝 Documentation Update
• [ ] 🎨 Style
• [ ] 🧑‍💻 Code Refactor
• [ ] 🔥 Performance Improvements
• [ ] ✅ Test
• [ ] 🤖 Build
• [ ] 🔁 CI
• [ ] 📦 Chore (Release)
• [ ] ⏩ Revert

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

Added tests?

• [ ] 👍 yes
• [ ] 🙅 no, because they aren't needed
• [x] 🙋 no, because I need help

Added to documentation?

• [ ] 📜 README.md
• [ ] 📓 docs.opensauced.pizza
• [ ] 🍕 dev.to/opensauced
• [ ] 📕 storybook
• [x] 🙅 no documentation needed

[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]]

✅ 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... Use your smartphone camera to open QR code link.

---

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

[0-vortex]

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

[bdougie]

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

[AlexVCS]

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

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]

No worries @AlexVCS. A few things.

• move the link to the right next to the submit (this will require a new div and around the items on the right)
• change to test-sm
• hide this entirely on mobile.

[AlexVCS]

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

[bdougie]

Mobile view for sign button is off

[AlexVCS]

Gotcha, should be fixed now!

[bdougie]

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

[AlexVCS]

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!

[bdougie]

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]

Thanks Brian! I made it a reusable component.

[0-vortex]

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! 🍕

[AlexVCS]

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:

Not signed in:

[0-vortex]

You are right, my mobile screen is too big 😓

[AlexVCS]

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]

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.

[AlexVCS]

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

[github-actions[bot]]

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

The release is available on:

• GitHub release
• ghcr.io container (@beta dist-tag)

Your semantic-release bot :package::rocket:

[github-actions[bot]]

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

The release is available on:

• GitHub release
• ghcr.io container (@latest dist-tag)

Your semantic-release bot :package::rocket: