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

[Feature] Add customization options #10

Merged
merged 5 commits into from
Jun 26, 2023

Conversation

mrcelo
Copy link
Contributor

@mrcelo mrcelo commented Jun 24, 2023

This PR adds customization options with original defaults (#1):

  • animationDuration
  • animationName
  • boxShadowInsetStart
  • boxShadowInsetEnd
  • highlightColor
  • outlineWidthStart
  • outlineWidthEnd
  • outlineStyle
  • enableAnimation
// tailwind.config.js
module.exports = {
  theme: {
    // ...
  },
  plugins: [
    require('tailwindcss-question-mark')({
      highlightColor: 'red',
      outlineStyle: 'dashed',
      enableAnimation: false,
    }),
  ],
}

@GavinJoyce
Copy link
Owner

GavinJoyce commented Jun 25, 2023

Thanks for the PR @mrcelo.

I wonder if we need all of these to be configurable? If we avoid exposing anything about box shadow or outline, we give ourselves the option of changing how this plugin works internally in future.

Perhaps these are the important ones?

  • color
  • widthStart & widthEnd (perhaps these would default to 8px and 12px?)
  • enableAnimation
  • animationDuration

What do you think?

* Removed unneeded props in keyframes
@mrcelo
Copy link
Contributor Author

mrcelo commented Jun 26, 2023

Those are all great suggestions, thanks for pointing that out. I've gone ahead and implemented these changes (and put a few constants in).

For my use case, 8px and 12px for the widths are a bit too much, but of course that's exactly the point of this PR! 😄

Let me know if LGTY. Thanks!

src/index.js Outdated
} = {}) => {
return function ({ addUtilities, e }) {
const BOX_SHADOW_INSET_START = "4px";
const BOX_SHADOW_INSET_END = "6px";
Copy link
Owner

Choose a reason for hiding this comment

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

So that the inset and outline are symmetrical, perhaps we should use widthStart and widthEnd for both?

If widthStart is 8px, we could apply 4px to both the inset and the outline. We'd need to do a little parsing of the string, but it might make for a nice api

What do you think?

Copy link
Owner

@GavinJoyce GavinJoyce Jun 26, 2023

Choose a reason for hiding this comment

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

Something like this?

function parsePx(input, defaultValue) {
  let value = input.match(/^\d+px$/);
  if (value) {
    return parseInt(value[0], 10);
  }
  return defaultValue;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely, that's a great idea. Small nitpick, but perhaps the regex /\d+px/ would better approximate browser behavior when parsing the CSS value (i.e. ignoring whitespaces or chars before or after the px substring). Would it be worth it to sanitize this far? How does that sound?

Copy link
Owner

Choose a reason for hiding this comment

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

Sounds good, thanks

src/index.js Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
* Added parsePx func
* Fixed typos
* Organized code
@GavinJoyce
Copy link
Owner

This is really great, thanks!

@GavinJoyce GavinJoyce merged commit c6f5078 into GavinJoyce:master Jun 26, 2023
@GavinJoyce
Copy link
Owner

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.

2 participants