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

Simplify CalendarDay dom #291

Merged
merged 2 commits into from
Feb 7, 2017

Conversation

moonboots
Copy link
Collaborator

to: @majapw @ljharb

Simplifies the dom/styles for calendar days from <td><div><span>31</span></div></td> to <td>31</td>. The appearance remains unchanged.


const propTypes = {
className: PropTypes.string.isRequired,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't want to be passing down className

@majapw
Copy link
Collaborator

majapw commented Feb 3, 2017

A lot of the motivation for the previous dom structure was to keep all the table stuff in one file, but that may not be worth it.

I really don't think we should expose className on any component since that's generally an antipattern to everything that we do.

@moonboots
Copy link
Collaborator Author

Thanks @majapw, I've removed the className prop and instead pushed the modifier logic into CalendarDay, including the modifier css styles, getModifiersForDay, and specs. I think this refactor makes sense even independent of the dom changes because modifiers operate on the day level, not the month.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 86.944% when pulling d7bbf89 on moonboots:simplify-calendar-day-dom into 1deaa6c on airbnb:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 86.944% when pulling d5cfcd7 on moonboots:simplify-calendar-day-dom into 1deaa6c on airbnb:master.

Copy link
Collaborator

@majapw majapw left a comment

Choose a reason for hiding this comment

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

This seems reasonable to me.

Hilariously, I'm pretty sure this is a return to code we had before. Whoops.

@majapw majapw merged commit 629d8c0 into react-dates:master Feb 7, 2017
@majapw majapw added the semver-patch: fixes/refactors/etc Anything that's not major or minor. label Feb 7, 2017
@timhwang21
Copy link
Collaborator

Can styling changes be considered 'breaking?' Just a note that this might break styling for anyone who has styled the day cells (e.g. me).

For what it's worth it's an easy fix, but I was very confused for a second as to why none of my styles worked any more (package had automatically updated from 6.0.1 to 6.0.2 without me noticing).

Cheers!

@majapw
Copy link
Collaborator

majapw commented Feb 8, 2017

Whoops! You know the thought had crossed my mind, but you are probably right in that this should ahve been marked as breaking. @ljharb do you have any advice?

@ljharb
Copy link
Member

ljharb commented Feb 9, 2017

If that's the case, we should revert it in the v6 line and publish a v6.0.3, and then re-revert it and publish a v7 (or, find a way to make it not breaking)

@JaKXz
Copy link
Contributor

JaKXz commented Feb 10, 2017

FWIW for nyc we use a strategy of publishing --tag next before going to @latest to help mitigate these risks by giving early adopters / contributors a chance to smoke test :) like @timhwang21 this was a surprise breakage for my team.

Thanks for getting back on it quickly though! 👍

majapw pushed a commit that referenced this pull request Feb 10, 2017
…dom"

This reverts commit 629d8c0, reversing
changes made to 6478599.
@majapw majapw added semver-major: breaking change A non-backwards-compatible change; anything that breaks code - including adding a peerDep. and removed semver-patch: fixes/refactors/etc Anything that's not major or minor. labels Feb 10, 2017
@majapw
Copy link
Collaborator

majapw commented Feb 10, 2017

Hi @JaKXz @timhwang21 I just released v6.1.0 which includes a revert of this change. I'll release v7.0.0 shortly!

majapw pushed a commit that referenced this pull request Feb 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-major: breaking change A non-backwards-compatible change; anything that breaks code - including adding a peerDep.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants