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.

Sign up for GitHub

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

Jump to bottom

Remove last-in-range style modifiers to fix border issue #1538

Merged
merged 3 commits into from
Feb 27, 2019
Merged

Remove last-in-range style modifiers to fix border issue #1538

merged 3 commits into from
Feb 27, 2019

Conversation

krissalvador27
Copy link
Contributor

@krissalvador27 krissalvador27 commented Feb 10, 2019 •
edited
Loading

Fixes issue #1536

Before

  • Border top of last day in selected range is not selected color

screen shot 2019-02-08 at 3 37 04 pm

  • Border of the hovered day overlaps border of the last day in selected range

screen shot 2019-02-09 at 4 41 49 pm

After

  • Border top of last day in selected range is expected selected color

screen shot 2019-02-08 at 3 36 44 pm

  • Border of the hovered day no longer overlaps border of the last day in selected range

screen shot 2019-02-09 at 4 41 35 pm

  • Not sure if it's an issue (I don't think it is) but the border of the last in range day overlaps the border of the last day, making it appear 1 pixel smaller than the first day. It was previously fixed with PR Changing border-styles to minimize border overlapping #1328 but I think that surfaced the issue this PR fixes now.

@coveralls
Copy link

coveralls commented Feb 10, 2019 •
edited
Loading

Coverage Status

Coverage remained the same at 84.504% when pulling 8f46272 on krissalvador27:remove-last-in-range-styles into 1a55d68 on airbnb:master.

ljharb
ljharb reviewed Feb 10, 2019
View reviewed changes
test/components/DayPickerRangeController_spec.jsx Outdated
@@ -2698,18 +2698,6 @@ describe('DayPickerRangeController', () => {
expect(modifiers.has('selected-span')).to.equal(true);
});

it('contains `last-in-range` if this.isLastInRange returns true', () => {
Copy link
Member

Choose a reason for hiding this comment

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

The removal of this test indicates it’s a breaking change; can we avoid that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can revert the code deleted on this.modifier but keep the deleted last in range check and styles add that caused the issue

@krissalvador27
Copy link
Contributor Author

@ljharb let me know if there's anything else you'd like me to do :)

ljharb
ljharb approved these changes Feb 11, 2019
View reviewed changes
src/components/CustomizableCalendarDay.jsx
@@ -160,14 +159,6 @@ export const selectedSpanStyles = {
},
};

export const lastInRangeStyles = {
Copy link
Member

Choose a reason for hiding this comment

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

to be really careful about breaking changes, we might want to keep this as export const lastInRangeStyles = {}

Copy link
Contributor

Choose a reason for hiding this comment

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

I think making lastInRangeStyles = {} would fix the bug without needing to completely remove the modifier's styles. This would make it so that there is no built-in styling for lastInRange, thus fixing the bug, but you could still supply your own styles if you wanted to.

@krissalvador27
Copy link
Contributor Author

@nkinser @ljharb Great catch! Removing lastInRangeStyles would have been a breaking change for sure. Made it an {} and rebased commits on master.

@ljharb ljharb added the semver-patch: fixes/refactors/etc Anything that's not major or minor. label Feb 12, 2019
@krissalvador27
Copy link
Contributor Author

@majapw anything else needed to get this merged in?

@majapw majapw merged commit ea0cf80 into react-dates:master Feb 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ljharb ljharb ljharb approved these changes

@monokrome monokrome monokrome approved these changes

@majapw majapw majapw approved these changes

@nkinser nkinser nkinser approved these changes

Assignees
No one assigned
Labels
semver-patch: fixes/refactors/etc Anything that's not major or minor.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@krissalvador27 @coveralls @ljharb @monokrome @majapw @nkinser