remove showWeekNumbers from DatePicker unsupportedProps

[tomitank]

closes #7668

[cla-checker-service]

💚 CLA has been signed

[github-actions]

👋 Since this is a community submitted pull request, a Buildkite build has not been started automatically. Would an Elastic organization member please verify the contents of this pull request and kick off a build manually?

[JasonStoltz]

buildkite test this

[cee-chen]

@tomitank You'll need to sign the CLA agreement with the email address tied to your git commits. You can use git log to see what that email address is.

[cee-chen]

@tomitank We cannot accept PRs without a signed contributor agreement. Moving this PR back into draft until one has been signed with the email address associated with the git commits.

[tomitank]

@tomitank You'll need to sign the CLA agreement with the email address tied to your git commits. You can use git log to see what that email address is.

I signed, what next? :)

[cee-chen]

buildkite test this

[cee-chen]

Thanks @tomitank - since this materially affects UI (screenshot below of showWeekNumbers with prop set to true), we'll need to have at least one designer review and approve it as a design decision that we want to make. We'll probably discuss this around next Monday.

[kibanamachine]

Preview staging links for this PR:

• Docs site: https://eui.elastic.co/pr_7725/
• Storybook: https://eui.elastic.co/pr_7725/storybook

[elasticmachine]

💚 Build Succeeded

• Buildkite Build
• Commit: 797bca9

History

• 💚 Build #1858 succeeded 897bd16

[MichaelMarcialis]

Thanks @tomitank - since this materially affects UI (screenshot below of showWeekNumbers with prop set to true), we'll need to have at least one designer review and approve it as a design decision that we want to make.

Thanks for the ping, @cee-chen. If we were to include this as a prop in EUI, I'd want to be sure that 1) it was off by default (which it sounds like it is) and 2) the week numbers were even more diminished and visibly divided than shown in your screenshot above. Perhaps something like the below example, which applies our lightest/disabled text color, replaces the gray background with dividing borders, and removes the # heading. CCing the other designers in @elastic/platform-design to see if they have any thoughts too.

That said, I suppose this brings up a larger operational question as to whether we should support this request. I personally can't think of an instance we'd want to use it in Kibana (or any other Elastic property using EUI), but there is obviously interest from external consumers. Do we wish to include prop options like this that aren't specifically used by Elastic in our design system? Or would doing so inadvertently encourage Elastic engineers to potentially use it (even if we don't want them to) or create opportunities for unintended inconsistency? Do our obligations lie primarily to supporting Elastic projects only or should we be giving equal weight to non-Elastic consumers with feature requests that will only be used externally? Do we have any thoughts on how we wish to operate in that regard?

[cee-chen]

the week numbers were even more diminished and visibly divided than shown in your screenshot above. Perhaps something like the below example, which applies our lightest/disabled text color

An annoying side note: we start running into color contrast / accessibility concerns at that point 😬

FWIW - I totally agree with you on concerns about encouraging use in Elastic products. Generally, while we're an open source library, Kibana remains our primary client/target and we want to set defaults for that while leaving enough escape hatches for other consumers. My 2c is I would consider a @ts-ignore and custom CSS a fully sufficient escape hatch.

[tomitank]

@cee-chen yes, i maked unique type to use this prop, but i think it's very popular feature (Kalendar Woche) in germany companies.
Honestly, without typescript all user can use this feature with or without any risk. This feature based on a 3 party library, so maybe should be nice to enable it.

[cee-chen]

Honestly, without typescript all user can use this feature with or without any risk.

Unfortunately, that isn't true. I had to update the CSS of EuiDatePicker in order for the week numbers to show with the correct font and styles (see 36a86db). Otherwise, it simply looks broken. And whenever styling comes into play, we do need a designer's input on things like this.

This feature based on a 3 party library, so maybe should be nice to enable it.

EUI is a branded and fairly opinionated design system intended primarily for Elastic usage - so our components deliberately do not allow all possible permutations of props/configurations. While we do try to be flexible enough to support a wide variety of consumers, there are some UX patterns we deliberately do not want to adopt. If an existing escape hatch exists for edge cases or non-Elastic consumers, we typically consider that 'good enough'.

I apologize that this wasn't investigated more thoroughly before asking you to open a PR - we'll work on communicating that more clearly in the future.

[tomitank]

Unfortunately, that isn't true. I had to update the CSS of EuiDatePicker in order for the week numbers to show with the correct font and styles (see 36a86db). Otherwise, it simply looks broken. And whenever styling comes into play, we do need a designer's input on things like this.

no offense: :)
..but if you want them not to be able to use it in .jsx files either, then these properties should be filtered (should not be forwarded) by the way i'm happy to do the style part as well.

I apologize that this wasn't investigated more thoroughly before asking you to open a PR - we'll work on communicating that more clearly in the future.

Not problem, i would find this function useful, but if it is not included, then "hacking" remains :)

[formgeist]

CCing the other designers in @elastic/platform-design to see if they have any thoughts too.

As for the design, I'd show Wk or similar title on the column to indicate what the numbers refer to. At the moment, it might come across as a horizontally scrolling calendar where we're previewing the previous month to the left. I understand the original implementation had a title #, but I'd be more clear as what the column represents.

[MichaelMarcialis]

An annoying side note: we start running into color contrast / accessibility concerns at that point 😬

As for the design, I'd show Wk or similar title on the column to indicate what the numbers refer to.

Updating example screenshot to address these two points.

[JasonStoltz]

Hey all. I think we should pause on this PR. I'm concerned about expanding the API for our DatePicker here to include a feature that we don't actually need. While it is nice to expose a feature of the underlying library, it would also force us to continue to support that if we were to ever change the underlying library, and it creates more surface area for us to support.

Unless we have a clear use case or need for this in Elastic, I think we should avoid introducing this.

I'll let this open for a little while longer to see if @MichaelMarcialis or @formgeist object, otherwise, we should plan to close this PR.

Big apology to @tomitank for this. We super appreciate your willingness to make contributions to our library.