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

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

Open AlexVCS opened this issue 1 year ago • 8 comments

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?

Aug 05 '22 17:08 AlexVCS

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.

Aug 05 '22 17:08 netlify[bot]

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

Aug 05 '22 21:08 0-vortex

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

Aug 08 '22 22:08 bdougie

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

Aug 09 '22 00:08 AlexVCS

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.

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

Aug 10 '22 16:08 bdougie

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

Aug 12 '22 17:08 AlexVCS

Mobile view for sign button is off

Aug 12 '22 17:08 bdougie

Gotcha, should be fixed now! Screen Shot 2022-08-12 at 3 02 20 PM

Aug 12 '22 19:08 AlexVCS

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

Aug 14 '22 02:08 bdougie

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!

Aug 14 '22 16:08 AlexVCS

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>
)

Aug 15 '22 06:08 bdougie

Thanks Brian! I made it a reusable component. Screen Shot 2022-08-15 at 7 01 50 AM

Aug 15 '22 11:08 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! 🍕

Aug 15 '22 13:08 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! 🍕

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

Aug 15 '22 14:08 AlexVCS

You are right, my mobile screen is too big 😓

Aug 15 '22 14:08 0-vortex

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.

Aug 15 '22 15:08 AlexVCS

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.