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

New labels prop #35

Closed
gpbl opened this issue Jul 29, 2015 · 12 comments
Closed

New labels prop #35

gpbl opened this issue Jul 29, 2015 · 12 comments

Comments

@gpbl
Copy link
Owner

gpbl commented Jul 29, 2015

A labels prop may help to implement title and aria-label attributes (see #33). Example:

// default labels
let labels = { "today": "Today" }
<DayPicker labels={labels} />

// localized, modifier specific labels
let modifiers = { "weekend": day => day.getDate() === 5 || day.getDate() === 6 }
let labels = { "weekend": { en: "Weekend", it: "Fine settimana", de: "Wochenende" }
<DayPicker labels={labels} modifiers={modifiers} />
@gpbl
Copy link
Owner Author

gpbl commented Jul 29, 2015

Not sure if call the prop dayLabels or just labels

@basham
Copy link

basham commented Jul 29, 2015

I'm a little concerned about having this project be opinionated about the best way to implement this feature. I could see situations in which both title and aria-label could co-exist on the same calendar. But if providing both options, the API could get rather messy.

I wonder if what's really needed is a hook to add any additional attributes to a given day element. That way, the author could determine how title, aria-label, or anything else not currently managed by this project could be integrated. Yes, this could be accomplished by having a custom day component render with these attributes on child elements, but it may be nicer having the ability to attach it directly on the day element to reduce unnecessary DOM nesting.

@basham
Copy link

basham commented Jul 29, 2015

In this way, localization concerns for this feature are offloaded to the implementation, instead of this project.

@gpbl
Copy link
Owner Author

gpbl commented Jul 29, 2015

I agree with your point. There's a renderDay prop rendering the content of the day cell. Maybe it would work better rendering the whole cell?

function renderDay(day, modifiers) {
  return <div title="MyCustomLocalizedTitle">{ day.getDate() }</div>;
}
render() {
  return <DayPicker renderDay={renderDay} />;
}

The component would still append the proper className.

@basham
Copy link

basham commented Jul 29, 2015

I like where that's going. But in that solution, you'd be responsible for adding additional element properties (key, className, tabIndex, role, onKeyDown, onMouseEnter, onMouseLeave, onClick, onTouchTap) after the renderDay function has worked. Meaning that you're enhancing and potentially overriding implementation details. Perhaps it would be better for all the element properties to be wrapped in an object and sent as a prepared set of attributes that the implementation has to deal with. Yes, the implementation could mess things up, but it would also be within the control of the implementation to fix it.

In this way, the implementation would be responsible for transferring the generated properties on the new element. And it gives opportunity for the props to be modified as needed.

function renderDay(day, props) {
  // Modify props, as needed.
  return <div {...props} title="MyCustomLocalizedTitle">{ day.getDate() }</div>;
}
render() {
  return <DayPicker renderDay={renderDay} />;
}

A potential good consequence of this is that internally, you could be using this same mechanism, and therefore, the project is dog-fooding its own API. Some pseudo code to explain what I mean:

// src/DayPicker.js
static defaultProps = {
  renderDay: (day, props) => (<div {...props}>{ day.getDate() }</div>)
}
renderDay(month, day) {
  // ...
  const { renderDay } = this.props;
  // Construct `props` object instead of applying properties directly on the JSX element.
  // Handle all modifier details.
  return renderDay(day, props);
}

@basham
Copy link

basham commented Jul 29, 2015

In extending this idea, I wonder how necessary some of the Day handlers are (onDayClick, onDayTouchTap, onDayMouseEnter, onDayMouseLeave), if they could be applied directly in this fashion during implementation. Could be a nice way to slim the API a bit.

@basham
Copy link

basham commented Jul 29, 2015

@gpbl
Copy link
Owner Author

gpbl commented Jul 29, 2015

Yes I don't see the need to explicitly pass the props argument to the renderDay function: we should be able to overwrite the props if not passed to the component.

Passing the whole day cell should also be "enough" backward compatible, because the dev already wraps the content into an element (or he/she passes just a string). I need to confirm this, in case we could add a deprecation warning.

PS. It already dog-feeds that prop :-)

@gpbl
Copy link
Owner Author

gpbl commented Jul 29, 2015

However I'd like to add default support for aria-label without the need of using renderDay. English-localized content is already present with the bulk component, and it is meant to be overridden by a custom implementation of LocaleUtils to support other languages. LocaleUtils could implement a getARIALabelForDay(day, modifiers, locale). Mmmhh

@basham
Copy link

basham commented Jul 29, 2015

Seems like a worthy endeavor. Just keep in mind that not every Day cell will need an aria-label value (which means the attribute doesn't need to be rendered in those cases). The original desire to have aria-label="Today" applied is because none of the other semantics (aria-disabled, aria-selected) sufficiently describe what is visually presented via the .DayPicker-Day--today modifier class. It is not necessary, and it may be even a detriment, if aria-label is overused. For example:

Do

<div
  class="DayPicker-Day DayPicker-Day--today DayPicker-Day--selected"
  aria-label="Today"
  aria-selected="true"
  ...>
  29
</div>
<div
  class="DayPicker-Day DayPicker-Day--selected"
  aria-selected="true"
  ...>
  30
</div>

Do not

<div
  class="DayPicker-Day DayPicker-Day--today DayPicker-Day--selected"
  aria-label="Today, Selected"
  aria-selected="true"
  ...>
  29
</div>
<div
  class="DayPicker-Day DayPicker-Day--selected"
  aria-label="Selected"
  aria-selected="true"
  ...>
  30
</div>

@gpbl
Copy link
Owner Author

gpbl commented Jul 29, 2015

Perfect thanks for the hint!

@gpbl
Copy link
Owner Author

gpbl commented Sep 18, 2016

I'm closing this as such values can be changed from a custom LocaleUtils prop.

@gpbl gpbl closed this as completed Sep 18, 2016
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

No branches or pull requests

2 participants