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 renderCalendarDay prop #894

Merged
merged 1 commit into from Dec 11, 2017
Merged

Add renderCalendarDay prop #894

merged 1 commit into from Dec 11, 2017

Conversation

majapw
Copy link
Collaborator

@majapw majapw commented Dec 9, 2017

Here are the breaking changes in this PR:

  • renderDay is now named renderDayContents
  • CalendarDay no longer consists of a button inside of a td. It is instead a td with a button role. The related styles have been simplified accordingly. FYI @backwardok

Here is what's new:

  • renderCalendarDay, a prop that takes a props arg and expects either a CalendarDay or CustomizableCalendarDay node with those props spread onto it to be returned. More details below.

===

This is for one-off, no-good, hacky customization of CalendarDay styling within the frame of the react-with-styles ecosystem. This PR isn't quite done, but it is close.

SO IN AN IDEAL WORLD, you only modify CalendarDay styles on an app-wide level, which react-with-styles themes allow you to do easily. After all, why would you have multiple datepickers that all look dramatically different on your site (especially with their color scheme). But sometimes, ya gotta do what ya gotta do. To solve this, we have a new beautiful renderCalendarDay prop.

The renderCalendarDay prop takes in an object (specifically, with the following keys---key, dayOfWeek, day, daySize, isOutsideDay, tabIndex, isFocused, onDayMouseEnter, onDayMouseLeave, onDayClick, renderDayContents, phrases, modifiers, ariaLabelFormat) as an argument. What it expects as a return is an instance of either CalendarDay (the default) or CustomizableCalendarDay. The reason the arg doesn't really matter is because you are kind of expected just to spread it onto the CalendarDay instance to get a fully functional datepicker.

The default renderCalendarDay prop looks as follow:

renderCalendarDay: props => <CalendarDay {...props} />

The CalendarDay is fairly restricted so you can't really do much more than that (as an aside, you could probably use this to hack into the day interaction methods from the DRP or SDP or the controllers). However, let's say you are trying to implement the following design:
screen shot 2017-12-08 at 7 54 41 pm
#PantoneUltraViolet

More importantly, let's say you are trying to implement the above design on one page of your app, AND EVERY OTHER PAGE HAS THE STANDARD TEAL. Well, if you're using react-with-styles-interface-aphrodite you might think you're kind of fucked. Aphrodite does not allow for arbitrary CSS overrides, and we have not before had any APIs that would allow for this. Now, you would just leverage the renderCalendarDay prop and the CustomizableCalendarDay component like so:

const selectedStyles = {
  background: '#590098',
  border: '1px solid #590098',
  color: '#fff',

  hover: {
    background: '#7A32AC',
    border: '1px solid #7A32AC',
    color: '#fff',
  },
};

const hoveredStyles = {
  background: '#cd99d0',
  border: '1px solid #cd99d0',
  color: '#fff',
};

const customDayStyles = {
  selectedStartStyles: selectedStyles,
  selectedEndStyles: selectedStyles,
  hoveredSpanStyles: hoveredStyles,
  afterHoveredStartStyles: hoveredStyles,

  selectedSpanStyles: {
    background: '#9b32a2',
    border: '1px solid #9b32a2',
    color: '#fff',

    hover: {
      background: '#83008b',
      border: '1px solid #83008b',
      color: '#fff',
    },
  },
};

function(props) {
  return (
    <DateRangePicker
      {...props}
      renderCalendarDay={props => <CustomizableCalendarDay {...props} {...customStyles} />}
    />
  );
}

And voila, you have a one-off, random ultraviolet datepicker in your app.

@ljharb Taylor Swift's "Look what you made me do" is now stuck in my head because that is how I feel about this PR and the unfortunate circumstances that have led to its necessity.

@airbnb/webinfra @amhunt if you feel like taking a gander at what I expect the customization API to look like

@majapw majapw added the semver-major: breaking change A non-backwards-compatible change; anything that breaks code - including adding a peerDep. label Dec 9, 2017
@majapw majapw force-pushed the maja-breaking-renderDay-change branch 2 times, most recently from 9b0a9cc to 2bb653d Compare December 9, 2017 04:02
@coveralls
Copy link

Coverage Status

Coverage decreased (-3.2%) to 82.953% when pulling 9b0a9cc on maja-breaking-renderDay-change into bab162d on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-3.3%) to 82.822% when pulling 2bb653d on maja-breaking-renderDay-change into bab162d on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-3.3%) to 82.822% when pulling 2bb653d on maja-breaking-renderDay-change into bab162d on master.

@@ -23,7 +23,7 @@ const propTypes = forbidExtraProps({
onDayClick: PropTypes.func,
onDayMouseEnter: PropTypes.func,
onDayMouseLeave: PropTypes.func,
renderDay: PropTypes.func,
renderDayContents: PropTypes.func,
Copy link
Member

Choose a reason for hiding this comment

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

hm - i'm not sure this prop actually has to change. Is the name that much clearer that it's worth the breaking change now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This change is already breaking because I've taken the opportunity to restructure the CalendarDay component. I think with the presence of a renderCalendarDay prop, renderDay is confusing and we should take this opportunity to clarify what that prop actually does.

Copy link
Member

Choose a reason for hiding this comment

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

master might already be breaking, but this PR doesn't have to be, and wouldn't be without this prop being renamed.

The naming confusion is a fair point, but CalendarDay doesn't take a renderCalendarDay prop - the prop names don't need to be the same across all our components.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry I think in my late night Friday shuffling around, I lost a change in this component that will most definitely making this PR breaking. I don't think master is breaking right now.

I maintain that renderDayContents is a much more meaningful prop name (in this component and in others) and is not going to be the only breaking change in this PR.

@@ -282,15 +283,15 @@ class DateRangePicker extends React.Component {
if (withPortal || withFullScreenPortal) {
return (
<Portal>
{this.renderDayPicker()}
{this.renderDayContentsPicker()}
Copy link
Member

Choose a reason for hiding this comment

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

isn't this still rendering a day picker?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Whoops! D: find and replace :X

}

renderDayPicker() {
renderDayContentsPicker() {
Copy link
Member

Choose a reason for hiding this comment

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

same here?

@majapw majapw force-pushed the maja-breaking-renderDay-change branch from 2bb653d to 829690c Compare December 11, 2017 18:31
@majapw
Copy link
Collaborator Author

majapw commented Dec 11, 2017

@ljharb take another look pls?

@coveralls
Copy link

Coverage Status

Coverage decreased (-3.5%) to 82.578% when pulling 829690c on maja-breaking-renderDay-change into bab162d on master.

ljharb
ljharb previously approved these changes Dec 11, 2017
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 85.705% when pulling d810291 on maja-breaking-renderDay-change into bab162d on master.

@majapw majapw merged commit 57fce50 into master Dec 11, 2017
@majapw majapw deleted the maja-breaking-renderDay-change branch December 11, 2017 22:24
@miloshevmitko
Copy link

In the styles i want to define for the CustomizableCalendarDay, how do i add styles for ::after ?
This is what I've tried so far:

const selectedStyles = {
  background: '#590098',
  border: '1px solid #590098',
  color: '#fff',

  after: {...},
  '&:after': {...},
  '::after': {...}
};

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.

None yet

4 participants