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

button lift and text shadow #80

Merged

Conversation

tof-tof
Copy link
Contributor

@tof-tof tof-tof commented Aug 19, 2021

added button lift on hover
added extra shadow to buttons to help it stand out from the cards
set the tracking on the buttons to be a bit wider as the text as upper case
added background shadow to directionary text

resolve #14

added button lift on hover
added extra shadow to buttons to help it stand out from the cards
set the tracking on the buttons to be a bit wider as the text as upper case
added background shadow to directionary text
@netlify
Copy link

netlify bot commented Aug 19, 2021

✔️ Deploy Preview for helpafamily-margarita-humanitarian ready!
Built without sensitive environment variables

🔨 Explore the source changes: 31201b3

🔍 Inspect the deploy log: https://app.netlify.com/sites/helpafamily-margarita-humanitarian/deploys/611eb11c4d6f97000770d5c2

😎 Browse the preview: https://deploy-preview-80--helpafamily-margarita-humanitarian.netlify.app

using margaritahumanitarian#79 fix to fix deploy errors
@RedFox0x20
Copy link
Collaborator

RedFox0x20 commented Aug 19, 2021

I like the hover effect, very clear interaction.

@audreyfeldroy
Copy link
Member

I love what you did here @tof-tof! These sorts of subtle style and UX improvements to delight the visitor are wonderful.

Thank you @RedFox0x20 for providing friendly guidance here. I appreciate you pitching in on the review side so much!

Yep, the best time to pull commits from the main branch is before the PR. The next best time is after you submit the PR and realize your branch is behind...nice work pulling and updating your PR, and fixing the lint error 🙂

I'm in meetings for awhile, but I'll follow up later with a more detailed technical review of your diff.

@@ -1,4 +1,5 @@
import 'tailwindcss/tailwind.css';
// import 'tailwindcss/tailwind.css';
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// import 'tailwindcss/tailwind.css';

@audreyfeldroy
Copy link
Member

The diff looks good. I'm still getting the hang of Tailwind CSS and am a bit tired, so I'd actually like to revisit the rest of this review tomorrow with fresh eyes.

@marekrozmus if you'd like, you're welcome to review and merge this before I return tomorrow.

Copy link
Collaborator

@marekrozmus marekrozmus left a comment

Choose a reason for hiding this comment

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

@audreyfeldroy I'm not huge fan of lifting buttons as we are changing here default behaviour and look and feel of daisyui but if you like it then go ahead 🙂

@@ -1,4 +1,5 @@
import 'tailwindcss/tailwind.css';
// import 'tailwindcss/tailwind.css';
import '../styles/global.css';
Copy link
Collaborator

@marekrozmus marekrozmus Aug 20, 2021

Choose a reason for hiding this comment

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

Please place local imports below the external modules like:

import PropTypes from 'prop-types';
import React from 'react';

import '../styles/global.css';

.shaded-text{
@apply bg-black bg-opacity-10 py-2 rounded-lg;
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove empty line

@audreyfeldroy
Copy link
Member

Thanks for the feedback about changing default daisyUI behavior @marekrozmus. It's a good point. Custom behavior does increase the maintenance effort on our side. I've given it serious consideration. My read of daisyUI is that it's like Bootstrap, where default components are provided but it's common/expected to customize the styles. https://daisyui.com/docs/customize looks like it encourages customization.

Here the lift and shadow behavior adds enough improvement that I'd like to accept this PR. It's also subtle enough that I see it as compatible with #76.

I do think it's the sort of change that could potentially be useful upstream to daisyUI. @tof-tof before I recommend that you submit an upstream PR:

@saadeghi Hey, would you be interested in a daisyUI PR for the button lift and/or text shadow in this PR? See it in action in the Netlify deploy preview above.

Copy link
Collaborator

@pydanny pydanny 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 to me.

@pydanny pydanny merged commit 8d9ece2 into margaritahumanitarian:main Aug 21, 2021
@pydanny
Copy link
Collaborator

pydanny commented Aug 21, 2021

@tof-tof Awesome PR! 👍🏻

@saadeghi
Copy link

@audreyfeldroy I encourage everyone to customize design decisions for their projects. Thanks to Tailwind's utility classes it's easy to customize things.

@tof-tof tof-tof deleted the adding-button-lift-on-hover branch August 23, 2021 15:17
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.

Improve design in any way
6 participants