-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP]: Refactor team page #94
[WIP]: Refactor team page #94
Conversation
✅ Deploy Preview for suspicious-boyd-b429fe ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @rennavittorio! There are some things that need to be updated before merging this PR. Please check the review comments that I have left you on some parts of the code and check that those rules are implemented PR-wide so we can proceed with a smooth merge.
I know some of this changes are tedious but we need to set a system that we can follow so whenever someone else submits a PR, we can all be on sync on how the specific stylings should be written and applied. This will save us a lot of work in the future if we have to do another refactoring like the one we are doing.
Remember, if you have any doubts or need help, just give us a heads up =] 🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does this image come from and what is its purpose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a possible idea to manage members without photos (ie. a placeholder), and accelerate the insertion of members profile into the website, just a propose, if we want to handle these cases differently, I'll align the page :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this idea! You can set it as default when on the component if no src
is passed to the img
.
I'm still seeing the Lorenzo Bugli image broken though!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's some confusion on how the UnoCSS rules are declared and utilized in this PR: for example, we have rules like: px-15px
, py-5px
, w-128px
,mx-2em
, text-1rem
utilized in this component which mix all the possible types of units that we can utilize in CSS. This plays against the flexibility of UnoCSS and their already baked in values in rem
units.
-
w-128px
shouldn't be utilized like this, because you could've already declared it likew-32
which if you multiply32 * 4 = 128
it will give you your desired result! You can also check this specific documentation of TailwindCSS width which allows you to see how all this type of frameworks have an approach of 4 point grid in their design. -
py-5px
should be avoided because it doesn't fit into the4pt grid
, you could easily usepy-1
to obtain4px
. -
px-15px
should be avoided as well, you could utilizepx-4
which would equal to16px
and and by doing that avoiding to use px on an UnoCSS rule. -
mx-2em
the use ofem
is discouraged as it doesn't fit well with the4pt grid
. You should stick torem
values. -
text-1rem
could also be rewritten astext-base
. See explicit font size documentation of Tailwind here. Even with the documentation read, you shouldn't be applying this rule because the global font value is already16px
(which equals to1rem
) check it out on your own App.vue file!
You can check out this recent component that has been updated to utilize UnoCSS so you can see how there's no need to implement all different types of unit values on the utility rules if we already stand by a system =]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix(components): align unocss classes to project, manage light-dark m…
fixed
(also big thanks for all the docs linked! <3 )
src/components/MemberCard.vue
Outdated
<SocialIcons :member="member" :socials="socials" /> | ||
|
||
<router-link | ||
class="w-fit px-15px py-5px border-1px border-solid border-transparent rounded-full bg-#586379 text-#fff text-1rem font-700 hover:bg-#2e3440" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to declare text-#fff
this is simply text-white
see Text Color on Tailwind docs.
This rule is complicated: hover:bg-#2e3440
what happens if one day we want to change this element into a component but we want to have different colours on hover? We would have to go to each component and modify it. Better to declare a global SCSS class that allows you to declare in the <style scoped lang="scss">
and their respective classes to apply it.
Also, this colours have already been declared on the nord.scss
file which is then imported globally so you could easily add them on a specific SCSS class:
<style scoped lang="scss">
.my-class {
&:hover {
background-color: $dark-bg-primary;
}
}
</style>
Also, does this component implement the change between light/dark theme properly?
Remember, refactoring to UnoCSS doesn't mean SCSS shouldn't be used at all!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
chore(config): add brand color to unocss config
What do you think about this solution, and moving project variables into unocss config?
Let's take full advantage of this cool tool! @Readpato
src/views/TeamPage.vue
Outdated
<h1 class="max-w-700px m-auto text-center text-2rem font-700" data-test="team-page-headline"> | ||
Schrödinger Hat's fam | ||
</h1> | ||
<div class="grid grid-cols-1 sm:grid-cols-2 lg:grid-cols-3 gap-20px p-1.5rem"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specific breakpoints rules sm:*
, lg:*
should be at the end of the string so we can quickly identify where they are
So this would be:
<div class="grid grid-cols-1 gap-20px p-1.5rem sm:grid-cols-2 lg:grid-cols-3">
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/> | ||
|
||
<router-link | ||
class="w-fit px-15px py-5px border-1px border-solid border-transparent rounded-full bg-#586379 text-#fff text-1rem font-700 hover:bg-#2e3440" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unify classes without utilizing px
and hex colours in line. See feedback left on other comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Closing this due to inactivity |
Changes
What kind of change does this PR introduce? (check at least one by adding an "x" between the brackets)
Does this PR introduce a breaking change? (check one)
The PR fulfills these requirements: