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

Allow users to set duration instead of end date #32 #47

Conversation

leonardohofling
Copy link

No description provided.

@friedger
Copy link
Member

friedger commented Feb 8, 2019

In general, please provide a description what this PR includes and what it doesn't

@friedger
Copy link
Member

friedger commented Feb 8, 2019

@leonardohof I think we need an extra property for calculatedEndTime so that the endTime is not set/overwritten when calculating from duration. And then the UI can show calculatedEndTime or endTime

This issue is mainly to deal with incoming events that have duration or endtime defined.

@friedger
Copy link
Member

friedger commented Feb 8, 2019

Can you please attach a quick screenshot of the UI?

@leonardohofling
Copy link
Author

@friedger Ok.
screenshot from 2019-02-08 09-10-59
screenshot from 2019-02-08 09-11-12

@friedger
Copy link
Member

friedger commented Feb 8, 2019

Looks good, we have to find a better UX, but that is another issue.
Have you looked at the import, export in ical format? Does that work?

@leonardohofling
Copy link
Author

@friedger I tried to import my google calendar, but none of my events appeared. And, i didn't see in Google Calendar the option to set a duration instead end date.
And, one last item: what CORS plugin are you using on Chrome/Firefox?

@friedger
Copy link
Member

friedger commented Feb 8, 2019 via email

@leonardohofling
Copy link
Author

leonardohofling commented Feb 8, 2019 via email

@friedger
Copy link
Member

friedger commented Feb 8, 2019

we are using https://www.npmjs.com/package/ics to create this field, so we only need to worry that this library gets the correct value. Limiting the duration to hours is fine for now.

@leonardohofling
Copy link
Author

leonardohofling commented Feb 8, 2019 via email

@leonardohofling
Copy link
Author

@friedger I did some changes in ical.js, to include the duration field in the import/export feature of ics files, following the ics documentation that you sent.

src/flow/io/ical.js Outdated Show resolved Hide resolved
src/flow/io/ical.js Outdated Show resolved Hide resolved
@friedger
Copy link
Member

friedger commented Feb 9, 2019

@leonardohof that looks good, I have two smaller comments.

Any ideas about adding a test?

@leonardohofling
Copy link
Author

@leonardohof that looks good, I have two smaller comments.

Any ideas about adding a test?
Yes, i fixed it.
So, about the tests, I'm trying to execute the actual tests using npm test and yarn test, but it's not working...

@friedger
Copy link
Member

@leonardohof I just pushed a fix for the App test in the develop branch. How would you write a test for the ical import/export?

@friedger
Copy link
Member

@leonardohof I haven't thought of the all day case. What could be a good behavior if the checkbox "all day" is checked on and off?

@friedger
Copy link
Member

@leonardohof I am on matrix now.

@leonardohofling
Copy link
Author

This causes the UI to move. Probably better to set the radio group to end date and disable the group

Done.

@friedger friedger merged commit f0a953f into openintents:develop Feb 11, 2019
@friedger
Copy link
Member

Thank you for the work

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.

None yet

2 participants