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(CalendarDay): apply styles for start and end modifiers #796

Merged
merged 1 commit into from
Oct 26, 2017
Merged

fix(CalendarDay): apply styles for start and end modifiers #796

merged 1 commit into from
Oct 26, 2017

Conversation

tagoro9
Copy link
Contributor

@tagoro9 tagoro9 commented Oct 25, 2017

Previous versions of the library would add the CalendarDay--selected-start and CalendarDay--selected-end class names whenselected-start and selected-end modifiers were passed.

I think after the switch to react-with-styles these classes got lost and making it harder to modify the styles of the beginning and end of a range.

I'm not very familiar with react-with-styles, so I don't know if this is the proper way to get those class names set in the component.

I would also like to add unit tests for this, but I didn't see tests in CalendarDay regarding other modifiers and I'm not quite sure how to test this. I see that the wrapper gets the class names in the test, but they have what it seems a random modifier at the end. Also, the react-with-styles docs mention that css(...) returns an object with an opaque structure, so definitely, checking the value of the props added by this object doesn't seem a good testing option.

Fixes #786.

Previous versions of the library would add class the
`CalendarDay--selected-start` and `CalendarDay--selected-end` when
`selected-start` and `selected-end` modifiers were passed.

Fixes #786.
@coveralls
Copy link

Coverage Status

Coverage remained the same at 86.76% when pulling a4b58d9 on relayrides:calendar_day_start_end_modifiers into ef98109 on airbnb:master.

@majapw
Copy link
Collaborator

majapw commented Oct 25, 2017

Hi @tagoro9, you should be able to test if the classes you expect get applied by running npm run storybook:css and inspecting the elements. The default storybook script uses aphrodite which is not quite... the class structure you are expecting.

@tagoro9
Copy link
Contributor Author

tagoro9 commented Oct 25, 2017

Thanks for the reply @majapw. I tested that the classes got added to the implementation in the story book. When I said I didn't know how to test it, I meant how to write a unit test that ensures that those classes get added. I should have explained that better in the description.

@ljharb ljharb added the semver-patch: fixes/refactors/etc Anything that's not major or minor. label Oct 25, 2017
@majapw
Copy link
Collaborator

majapw commented Oct 26, 2017

Sweet. Yeah, in general, we don't want to write tests for the styling (because it would be testing the interface's internals) and the end-goal is to integrate with a tool like https://github.com/Galooshi/happo instead to check for visual regressions.

THIS IS ON OUR ... eventual roadmap. :) but i've just been swamped.

@majapw majapw merged commit 56010cd into react-dates:master Oct 26, 2017
@tagoro9 tagoro9 deleted the calendar_day_start_end_modifiers branch October 26, 2017 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch: fixes/refactors/etc Anything that's not major or minor.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants