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

[website] Add Join Us block #24521

Merged
merged 3 commits into from
Jan 21, 2021

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Jan 20, 2021

@oliviertassinari oliviertassinari added the website Pages that are not documentation-related, marketing-focused. label Jan 20, 2021
@mui-pr-bot
Copy link

mui-pr-bot commented Jan 20, 2021

Details of bundle changes

Generated by 🚫 dangerJS against aa645b4

);
}

function BrandingSupportUs() {
Copy link
Member

Choose a reason for hiding this comment

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

👍

If you love the challenge of doing things differently, empowering creativity, and making
real connections along the way–then this may be the place for you.
</Typography>
<Button
Copy link
Member

Choose a reason for hiding this comment

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

image

Shouldn't the button look like this? Or maybe I am not up to date with the latest version.

Copy link
Member Author

@oliviertassinari oliviertassinari Jan 21, 2021

Choose a reason for hiding this comment

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

It's the only white button I could find in the whole design. I went on assuming that it was inconsistent and removed the variant. But not sure. It could also be an interesting customization exercise. Let's wait for Matt's point of view.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds fair :)

Copy link
Member

Choose a reason for hiding this comment

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

The blue definitely looks better, and +1 for consistency.

The only feedback I have is how laggy the hover and click styles feel on the buttons.

Copy link
Member Author

@oliviertassinari oliviertassinari Jan 21, 2021

Choose a reason for hiding this comment

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

The only feedback I have is how laggy the hover and click styles feel on the buttons.

@mbrookes The transition is from the default Material Design styles. It's 250ms. Bootstrap has 150ms. Tailwind 200ms. Chakra 250ms, GitHub 200ms.

I have reduced the color gap, it helps.

Otherwise, we could consider theme.transitions.duration.shorter (200ms) instead of theme.transitions.duration.short (250ms) for button base, but it's a large change and I would advocate shouldn't be specific to our custom branding.

Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

Looks good, just double check the Button styles please.

@oliviertassinari oliviertassinari merged commit 3979161 into mui:next Jan 21, 2021
@oliviertassinari oliviertassinari deleted the branding-one-block branch January 21, 2021 20:03
@@ -208,14 +208,14 @@ theme = createMuiTheme(theme, {
boxShadow: `0 0 0 0.25rem ${alpha(theme.palette.greyD7, 0.5)}`,
},
'&:hover': {
backgroundColor: darken(theme.palette.greyD7, 0.1),
backgroundColor: darken(theme.palette.greyD7, 0.07),
Copy link
Member

Choose a reason for hiding this comment

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

"reduce lag feeling"

Uh, how?

Copy link
Member Author

@oliviertassinari oliviertassinari Jan 21, 2021

Choose a reason for hiding this comment

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

Well, if you don't feel the difference, then it's a false attempt. I still do hope that the new opacities look better.

Oh no actually GitHub do 100ms for the hover-in and 200m for the hover-out transitions. Interesting tradeoff, but maybe too hard to customize and feel "unbalanced".

Copy link
Member

@mbrookes mbrookes Jan 21, 2021

Choose a reason for hiding this comment

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

Well, if you don't feel the difference, then it's a false attempt.

I didn't compare them side by side, but before looking at the code I couldn't notice a difference. And since changing the opacity doesn't change the transition duration, I'm not surprised. 🙂

We can pick it up in a mop-up / final review when the page is done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
website Pages that are not documentation-related, marketing-focused.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants