-
Notifications
You must be signed in to change notification settings - Fork 6
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
Upgrade v2.11.0 date picker #219
Conversation
e9e5c0d
to
974f3e3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks pretty nifty, good job @tomnez! Couple of minor comments/questions but otherwise GTG 👍
@@ -10,6 +11,8 @@ import { | |||
getNextDisplayMonth, | |||
getPreviousDisplayYear, | |||
getPreviousDisplayMonth, | |||
weekdays, // obj |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the // obj
comment needed here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah! That was supposed to be a temporary reminder to myself since Polaris uses enums for weekdays haha. Will remove..
@@ -10,6 +11,8 @@ import { | |||
getNextDisplayMonth, | |||
getPreviousDisplayYear, | |||
getPreviousDisplayMonth, | |||
weekdays, // obj | |||
isSameDay, | |||
} from '../utils/dates'; | |||
|
|||
// TODO: add changes for Polaris v2.2.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How 'bout these guys? Are those changes present now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah this can be removed. Ironically the additions from 2.0->2.2 are stripped out in 2.11 (other than translations on accessibility labels which we don't have)
* @type {String} | ||
* @default 'Sunday' | ||
*/ | ||
weekStartsOn: weekdays.Sunday, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This guy should probs be up with the rest of the public interface 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andrewpye Ah meant to comment on this, the linter hates these types of properties (it considers this an "unknown property type") and wants everything above it. I tried ignoring with eslint and prettier comments but it didn't seem to work. Any ideas on working around this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately I don't, but maybe @sivakumar-kailasam does?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like assigning to a variable first fixes it. Kinda wack, but better than having it at the bottom all willy nilly (I think)
3ade81f
to
12a916e
Compare
Diff here
Finishes #198