-
Notifications
You must be signed in to change notification settings - Fork 8
Adapt to DataValuesJavaScript 0.7.0 #162
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
Conversation
|
Almost +2, looks very good. I tried to fix the tests. @snaterlicious, can you please have a look and tell me if this is ok? I will merge it then. |
|
OK, cool, thanks. Sorry for forgetting about those tests... Fine with it. |
|
@snaterlicious, cool, thanks. Now this kind of depends on wmde/DataValuesJavaScript#69. Can you have a look? |
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 calls toLowerCase on an object or, with the simplified CALENDARS structure, on a string URI.
265c2f9 to
89ebed9
Compare
|
@snaterlicious, I amended my commit, adapted to the changes in wmde/DataValuesJavaScript#69 and found and fixed a small issue you missed (check the |
f4aadba to
30687e0
Compare
|
Wasn't that because of your amendment in DataValuesJavaScript? Whatever, looks good. |
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.
Where does a calendarModelcalendarName option come from? I think it doesn't exist and needs to be replaced with a TimeValue.getCalendarModelKeyByUri call.
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.
So, you did not test the pull request after your additional changes in DataValuesJavaScript? What do you expect value.getOption( 'calendarModel' ) to return being aware that value is a TimeValue object (https://github.com/wmde/DataValuesJavascript/blob/master/src/values/TimeValue.js)? If it is nor the URI, something is wrong as the call to TimeValue.getCalendarModelKeyByUri is just two lines below.
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 put the comment on the wrong line, see the calendarName option below.
dbdffee to
929a808
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 was referring to this line. I fixed it in my commit, see 929a808#diff-49060d53e8ad9e0d74b8e4ea672589a5L53.
|
rebased, updating version to 0.15.0 |
README.md
Outdated
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.
"DataValues JavaScript" please.
|
What's the reason for the change in the version number? |
|
when using this patch together with current versions of stuff (e.g. data values javascript), I get "Uncaught Error: Unknown dependency: dataValues.TimeValue" thus I think this is a breaking change. |
|
jquery.valueview.ExpertExtender.CalendarHint (or at least its tests) needs adjustments "jquery.valueview.ExpertExtender.CalendarHint: switch switches the calendar model" and |
|
Right, From semver.org: "What should I do if I update my own dependencies without changing the public API? In my opinion this classifies as a bugfix. |
|
@filbertkm I squashed the two commits and decreased the version number to 0.14.4. Is this ok for you? |
2ebeed1 to
4a469c3
Compare
4a469c3 to
23be0b8
Compare
Adapt to DataValuesJavaScript 0.7.0
No description provided.