Skip to content
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

fix: fixing color of buttons #5179

Merged
merged 13 commits into from Nov 14, 2022
Merged

fix: fixing color of buttons #5179

merged 13 commits into from Nov 14, 2022

Conversation

aballerr
Copy link
Contributor

@aballerr aballerr commented Nov 11, 2022

https://uniswaplabs.atlassian.net/browse/WEB-2159
https://uniswaplabs.atlassian.net/browse/WEB-2104

Fixing a bunch of button text colors on light mode
Also fixing royalty fees

@vercel
Copy link

vercel bot commented Nov 11, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
interface ✅ Ready (Inspect) Visit Preview Nov 14, 2022 at 8:58PM (UTC)


:hover {
color: ${({ theme }) => theme.accentActive};
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we had a random blue hoverstate

@@ -8,7 +8,8 @@ export const container = style([
paddingTop: '4',
}),
{
width: '300px',
width: '308px',
paddingRight: '8px',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the shadow on the right slider overlaps the adjacent div, which creates a line to the right that looks strange(light mode only). This maintains the original dimensions while solving that issue

@@ -41,7 +41,6 @@ export const commonButtonStyles = style([
sprinkles({
borderRadius: '12',
transition: '250',
boxShadow: { hover: 'elevation' },
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

common button styles has a boxShadow. Not a heavily used style, but we removed boxShadows from buttons

@aballerr aballerr requested review from a team and lynnshaoyu and removed request for a team November 11, 2022 22:20
@aballerr aballerr marked this pull request as ready for review November 11, 2022 22:21
Copy link
Collaborator

@unipadmini unipadmini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to hardcode white vs use design system colors?

@@ -37,7 +37,7 @@ export const FilterButton = ({
width={isMobile ? '44' : 'auto'}
height="44"
whiteSpace="nowrap"
color="textPrimary"
color="white"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we sure we want to hardcode this tow hite?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a merge conflict, i changed this upstream

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screen Shot 2022-11-14 at 9 40 02 AM

This is what it looks like with textPrimary

Copy link
Collaborator

@unipadmini unipadmini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments. (I think it might also be easy to split out the number formatting change in a different PR since I think the colors change is a bit tricker)

borderRadius: '4px',
border: 'none',
boxShadow: `${theme.shallowShadow.slice(0, -1)}`,
boxShadow: colorsDark.shallowShadow.slice(0, -1),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why aren't we pulling this from theme anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

theme has a different shadow color. I couldn't find this exact shadow defined anywhere but in darktheme.

@vm vm requested review from vm and removed request for lynnshaoyu and a team November 14, 2022 14:16
@@ -372,7 +372,7 @@ export const AssetPriceDetails = ({ asset, collection }: AssetPriceDetailsProps)
}
}}
>
<SubHeader lineHeight={'20px'}>
<SubHeader color="white" lineHeight={'20px'}>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be primaryColor?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

primary isn't white in light theme

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aballerr could you sync with yushin on this? Apparently they defined a button-label in the design system that reflects what the text colors should be for light and dark.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screen Shot 2022-11-14 at 11 03 31 AM
Screen Shot 2022-11-14 at 11 03 38 AM

This is what buy now currently looks like and this is what buttons on our site look like. I'm not really sure what I need to sync on to be honest


@media screen and (max-width: ${MOBILE_MEDIA_BREAKPOINT}) {
padding-left: 16px;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@vm vm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approving, but i feel like we should be avoiding using specific colors vs theme variables

@aballerr aballerr merged commit 28be15e into main Nov 14, 2022
@aballerr aballerr deleted the WEB-2159-button-color branch November 14, 2022 21:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants