added currentDate matcher#41
Conversation
|
|
||
| class CurrentDateMatcher { | ||
| isSatisfiedBy(prefix, name) { | ||
| return prefix === 'f' && name === 'currentDate'; |
There was a problem hiding this comment.
We're using prefix f for functions like isVisible, isClickable and so on. Maybe, in this case, it would be better to use a prefix m? What do you think @sethii ?
There was a problem hiding this comment.
Yeah actually might be better, this way
@edit i decided to go back to f:
There was a problem hiding this comment.
Okay, let's keep it with f:currentDate
| } | ||
|
|
||
| match(element) { | ||
| let currentDate = Sugar.Date.create('now'); |
There was a problem hiding this comment.
-
It's simple, so maybe you should use it like this:
const currentDate = Sugar.Date.format(Sugar.Date.create('now'), '{yyyy}-{MM}-{dd}'); -
Currently, you're supporting only
{yyyy}-{MM}-{dd}, I think it should be working in the same way as date comparator. So a user can provide datesdd/mm/yyyyand so on.
There was a problem hiding this comment.
- Yeah good point
- It supports all data formats, Sugar library can handle it, it's just changed to this format to compare by dates. https://sugarjs.com/dates/
| let compareDate = Sugar.Date.create(text); | ||
| compareDate = Sugar.Date.format(compareDate, '{yyyy}-{MM}-{dd}'); | ||
|
|
||
| if (compareDate === currentDate) { |
There was a problem hiding this comment.
Maybe you could use ES6 if...else
(compareDate === currentDate) ? true : false;
| import Sugar from 'sugar-date'; | ||
| import { expect } from 'chai'; | ||
|
|
||
| describe('Current Date matcher', () => { |
There was a problem hiding this comment.
You're missing tests which cover:
- dates with slash e.g. 2018/03/2018
There was a problem hiding this comment.
added test for incorrect date
…improvements for current date matcher
|
|
||
| class CurrentDateMatcher { | ||
| isSatisfiedBy(prefix, name) { | ||
| return prefix === 'f' && name === 'currentDate'; |
There was a problem hiding this comment.
Okay, let's keep it with f:currentDate
|
|
||
| return element.getText().then((text) => { | ||
| const compareDate = Sugar.Date.format(Sugar.Date.create(text), dateFormat); | ||
| return (compareDate === currentDate) ? true : false; |
There was a problem hiding this comment.
Maybe you can use just return compareDate === currentDate?
| Given I visit the "main" page | ||
| When I click the "matchersLink" element | ||
| Then the "matchers" page is displayed | ||
| And there is element "dateElement" with value "f:currentDate:{yyyy}-{MM}-{dd}" |
There was a problem hiding this comment.
Are you sure that we should be using curly braces in the feature files?
currently: "f:currentDate:{yyyy}-{MM}-{dd}"
proposition: "f:currentDate:yyyy-MM-dd"
@sethii What do you think?
There was a problem hiding this comment.
This is library property to pass format LDML tokens https://sugarjs.com/dates/#/Formatting
There was a problem hiding this comment.
Don't you have an established format od dates used in the project? I mean I guess there's a dictionary that says "here we use date with format: YY-mm-dd" or whatever else.
There was a problem hiding this comment.
As far as I know, sugar has something like global date format (in some cases we changed it into DD/MM/YYYY).
Maybe instead of providing format as a parameter, we could integrate it into global specified format ? This ways we will make sure that the date format is the same in every date related code.
|
Library was changed from sugar to moments, to remove {} brackets, also I added new functional tests. |
4 participants
No description provided.