hot added custom tailwind variables and removed anti patterns

added custom tailwind variables and removed anti patterns

Open AlexVCS opened this issue 1 year ago • 8 comments

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

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

Description

I added a few custom Tailwind variables and removed more of the anti patterns in Hero.tsx.

This PR is a work in progress fix for #315

Related Tickets & Documents

Fixes #315

Mobile & Desktop Screenshots/Recordings

Added tests?

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

Added to documentation?

[ ] 📜 README.md
[ ] 📓 docs.opensauced.pizza
[ ] 🍕 dev.to/opensauced
[ ] 📕 storybook
[✅ ] 🙅 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?

kob

Sep 10 '22 23:09 AlexVCS

Deploy Preview for hot-sauced-ui ready!

Name	Link
Latest commit	7d5a6f63d87e47e16a4e6fb701de6c9996e9e620
Latest deploy log	https://app.netlify.com/sites/hot-sauced-ui/deploys/6320d6bb51cc670008a1d5d7
Deploy Preview	https://deploy-preview-337--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.

Sep 10 '22 23:09 netlify[bot]

Hey @AlexVCS thanks for opening the PR, however this is moving the hardcoding from pixels to pixel css classes - simply put, none of the components were initially implemented as mobile first, which brings us to a responsive predicament when tailwind is a mobile first framework; as such, the baseline tailwind classes are defined in responsive units.

As per the above limitations, tailwind classes should be applied when possible, when not possible there are multiple things to try:

aproximating to the closest available tailwind class
investigating fixing the component
keep the hardcoding but switch from pixels to responsive units

Adding additional tailwind classes based on faulty implementations is not in the above list because it leads to more terrible complications. I would not close the PR as some of the work is compatible with the requirement, however the custom sizing needs to be undone 😅

Sep 10 '22 23:09 0-vortex

Hey @AlexVCS thanks for opening the PR, however this is moving the hardcoding from pixels to pixel css classes - simply put, none of the components were initially implemented as mobile first, which brings us to a responsive predicament when tailwind is a mobile first framework; as such, the baseline tailwind classes are defined in responsive units.

As per the above limitations, tailwind classes should be applied when possible, when not possible there are multiple things to try:

aproximating to the closest available tailwind class

investigating fixing the component

keep the hardcoding but switch from pixels to responsive units

Adding additional tailwind classes based on faulty implementations is not in the above list because it leads to more terrible complications. I would not close the PR as some of the work is compatible with the requirement, however the custom sizing needs to be undone 😅

Thanks for letting me know! I tried it as I saw #267 talked about potentially creating variables. My apologies, I will remove them and take your advice on how to proceed!

Sep 11 '22 00:09 AlexVCS

Would the shadow on line 61 be considered an anti pattern?

<div className="mt-11 px-4 gap-x-2.5 py-2.5 justify-between bg-white shadow-[0_35px_60px_-15px_rgba(0,0,0,0.3)] rounded-2xl md:min-w-[26.375rem] flex">

Thanks!

Sep 11 '22 20:09 AlexVCS

Would the shadow on line 61 be considered an anti pattern?

<div className="mt-11 px-4 gap-x-2.5 py-2.5 justify-between bg-white shadow-[0_35px_60px_-15px_rgba(0,0,0,0.3)] rounded-2xl md:min-w-[26.375rem] flex">

Thanks!

Technically yes, however in this particular case, https://tailwindcss.com/docs/box-shadow only shadow-2xl would be a viable replacement and I am not sure if that is visually satisfying, however I would personally go with shadow-2xl or lower one to keep consistency! 🍕

Sep 12 '22 21:09 0-vortex

Would the shadow on line 61 be considered an anti pattern? <div className="mt-11 px-4 gap-x-2.5 py-2.5 justify-between bg-white shadow-[0_35px_60px_-15px_rgba(0,0,0,0.3)] rounded-2xl md:min-w-[26.375rem] flex"> Thanks!

Technically yes, however in this particular case, https://tailwindcss.com/docs/box-shadow only shadow-2xl would be a viable replacement and I am not sure if that is visually satisfying, however I would personally go with shadow-2xl or lower one to keep consistency! 🍕

Thank you! I went with that and it looks pretty good:

Sep 13 '22 19:09 AlexVCS

Would the shadow on line 61 be considered an anti pattern? <div className="mt-11 px-4 gap-x-2.5 py-2.5 justify-between bg-white shadow-[0_35px_60px_-15px_rgba(0,0,0,0.3)] rounded-2xl md:min-w-[26.375rem] flex"> Thanks!

Technically yes, however in this particular case, https://tailwindcss.com/docs/box-shadow only shadow-2xl would be a viable replacement and I am not sure if that is visually satisfying, however I would personally go with shadow-2xl or lower one to keep consistency! 🍕

Thank you! I went with that and it looks pretty good:

Looks good to me! 👍

Sep 13 '22 20:09 0-vortex

@AlexVCS let us know when this is ready to review.