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

Add Support for custom styles #44

Merged

Conversation

PatrickDesign
Copy link
Collaborator

@PatrickDesign PatrickDesign commented Oct 3, 2020

Add 5 new props:

attribute default description
inputStyle {} Styles passed directly to the input element.
inputWrapperStyle {} Styles passed directly to the input wrapper div.
listItemStyle {} Styles passed to each item in the dropdown list.
listWrapperStyle {} Styles passed directly to the dropdown wrapper.
selectedListItemStyle {} Styles passed directly to current 'active' item.

Closes #42

/>,
);

expect(wrapper.find('input')).to.exist;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Could use some help making this assertion more helpful.. Was having trouble checking for style props.

Copy link
Owner

Choose a reason for hiding this comment

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

I think we should not invest time here and get rid of enzyme altogether since it's no longer maintained. We can use react-testing-library as a replacement.

@PatrickDesign PatrickDesign force-pushed the patrickW/feature--support-style-props branch from 9ff233a to 3e98ae2 Compare October 3, 2020 21:50
@ritz078
Copy link
Owner

ritz078 commented Oct 4, 2020

@PatrickDesign is this ready to be reviewed? Please request a review so that it's clear that this can be reviewed.

@PatrickDesign PatrickDesign requested a review from ritz078 October 4, 2020 20:41
@PatrickDesign
Copy link
Collaborator Author

Ready for review :)

Let me know if you think of an easier way to support custom styles - maybe ditch using the style prop and apply our base styles via a class.

Copy link
Owner

@ritz078 ritz078 left a comment

Choose a reason for hiding this comment

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

Let me know if you think of an easier way to support custom styles - maybe ditch using the style prop and apply our base styles via a class.

For libraries, I like using style objects instead of classNames because you don't have to setup CSS or CSS in JS in your app to use the styling if you are using the object form. Also it's better for dynamic styling.

This is great work. Thank you for doing this. I have left a single comment that might need action.

README.md Outdated
Comment on lines 69 to 71
resultsStyle|{}|Styles passed to each `result` in the dropdown list.
resultsWrapperStyle|{}|Styles passed directly to the dropdown wrapper.
selectedResultStyle|{}|Styles passed directly to current 'active' item.
Copy link
Owner

Choose a reason for hiding this comment

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

Can we use more generic term like list instead of result? Sometimes they might be suggestions instead of results so U think it makes sense to have more generic names.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated 🙇

  • resultsStyle -> listItemStyle
  • selectedResultStyle -> selectedListItemStyle
  • resultsWrapperStyle -> listWrapperStyle

@PatrickDesign PatrickDesign force-pushed the patrickW/feature--support-style-props branch 3 times, most recently from dd67738 to a416749 Compare October 6, 2020 06:00
Group styles together in README.
@PatrickDesign PatrickDesign force-pushed the patrickW/feature--support-style-props branch from a416749 to e1a6943 Compare October 6, 2020 06:03
@PatrickDesign PatrickDesign merged commit 0d20bf6 into ritz078:master Oct 6, 2020
@PatrickDesign PatrickDesign mentioned this pull request Oct 9, 2020
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.

Styling
2 participants