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

Fix 'today' in the datepicker #668

Merged
merged 1 commit into from
May 10, 2020
Merged

Fix 'today' in the datepicker #668

merged 1 commit into from
May 10, 2020

Conversation

shankari
Copy link
Contributor

This involved two fixes:

  • recreating the config on resume so that the end date is reset (list.js), and
  • setting "today" to the correct date when the datepicker is opened
    (ionic-datepicker.bundle.min.js)

I have also added build instructions to my fork of the ionic-datepicker repo,
but in case I delete the fork later, the instructions are:

Install node 10.19.0 (gulp-sass will fail for more recent version of node)

node -v
v10.19.0

Install and check gulp version

npm install
npm list | grep gulp
├─┬ [email protected]

Build

npx gulp build

Testing done:

Followed the steps in e-mission/e-mission-docs#531 (comment)
Both end date and today are updated correctly

This involved two fixes:
- recreating the config on resume so that the end date is reset (`list.js`), and
- setting "today" to the correct date when the datepicker is opened
    (`ionic-datepicker.bundle.min.js`)

I have also added build instructions to my fork of the ionic-datepicker repo,
but in case I delete the fork later, the instructions are:

Install node 10.19.0 ([gulp-sass will fail for more recent version of node](nodejs/node-gyp#1763))

```
node -v
v10.19.0
```

Install and check gulp version

```
npm install
npm list | grep gulp
├─┬ [email protected]
```

Build

```
npx gulp build
```

Testing done:

Followed the steps in e-mission/e-mission-docs#531 (comment)
Both end date and today are updated correctly
@shankari
Copy link
Contributor Author

@asiripanich can you please review this? I will then merge and deploy. Please test so that I can merge to master.

@asiripanich
Copy link
Member

sure!

@shankari
Copy link
Contributor Author

@asiripanich is that a review?

@asiripanich
Copy link
Member

@shankari I dont have the dev environment setup on my machine and @atton16 is not reachable.

I’m afraid this is going to take time if we have to wait for my review. Is it possible to merge and test instead?

@shankari
Copy link
Contributor Author

shankari commented May 10, 2020

@asiripanich by review, I mean look at the code. I already tested it (see the "testing done" in the commit). An external code review is almost always a good idea.

@asiripanich
Copy link
Member

Looks good to me.

@asiripanich
Copy link
Member

Could you please rebase and merge this pr?

@shankari
Copy link
Contributor Author

why do I need to rebase? It is already to rciti1...

@asiripanich
Copy link
Member

Ahh sorry my bad..

@shankari shankari merged commit 99418a3 into e-mission:rciti1 May 10, 2020
@asiripanich
Copy link
Member

Tried the testing steps on my device and can report that the issue no longer exists.

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

Successfully merging this pull request may close these issues.

2 participants