The Wayback Machine - https://web.archive.org/web/20201108164710/https://github.com/ng-bootstrap/ng-bootstrap/pull/3731
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

feat(datepicker): added week number header label + i18n #3731

Open
wants to merge 3 commits into
base: master
from

Conversation

@koxkox
Copy link

@koxkox koxkox commented May 14, 2020

Before submitting a pull request, please make sure you have at least performed the following:

  • read and followed the CONTRIBUTING.md and DEVELOPER.md guide.
  • built and tested the changes locally.
  • added/updated any applicable tests.
  • added/updated any applicable API documentation.
  • added/updated any applicable demos.
Stefan Panko added 2 commits May 14, 2020
@codecov-io
Copy link

@codecov-io codecov-io commented May 14, 2020

Codecov Report

Merging #3731 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3731   +/-   ##
=======================================
  Coverage   91.34%   91.35%           
=======================================
  Files         100      100           
  Lines        2971     2972    +1     
  Branches      548      548           
=======================================
+ Hits         2714     2715    +1     
  Misses        190      190           
  Partials       67       67           
Flag Coverage Δ
#e2e 56.12% <0.00%> (-0.02%) ⬇️
#unit 88.25% <100.00%> (+<0.01%) ⬆️
Impacted Files Coverage Δ
src/datepicker/datepicker-month.ts 100.00% <ø> (ø)
src/datepicker/datepicker-i18n.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ea59f88...2d37ec3. Read the comment docs.

@koxkox koxkox marked this pull request as ready for review May 14, 2020
@koxkox koxkox changed the title feat(datepicker): added ween number header label + i18n feat(datepicker): added week number header label + i18n May 16, 2020
Stefan Panko
Copy link
Member

@benouat benouat left a comment

Hey @koxkox

I did a first quick pass on your PR, and before continuing, I'd like to address a comment.

What is the actual requirement behind your PR ?
Do you need, visually speaking, to have a label displayed here in your application ?
Is it something related to a11y ? You need a screen reader to read it ?

I am just trying here to understand the change. I am hesitating and not convinced that we should generate it as a standalone text-node as compared to just having an aria-label. Especially that default implementation would be empty string.

@koxkox
Copy link
Author

@koxkox koxkox commented May 18, 2020

Hey @koxkox

I did a first quick pass on your PR, and before continuing, I'd like to address a comment.

What is the actual requirement behind your PR ?
Do you need, visually speaking, to have a label displayed here in your application ?
Is it something related to a11y ? You need a screen reader to read it ?

I am just trying here to understand the change. I am hesitating and not convinced that we should generate it as a standalone text-node as compared to just having an aria-label. Especially that default implementation would be empty string.

Hi @benouat

my goal was to add an option to display the week number column header and also make it localizable, just like the day/month names. The reason I kept it an empty string was to not break the default appearance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.