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 event date view #643

Merged
merged 36 commits into from Feb 14, 2020
Merged

Fix event date view #643

merged 36 commits into from Feb 14, 2020

Conversation

nileshgulia1
Copy link
Member

@nileshgulia1 nileshgulia1 commented Feb 22, 2019

@sneridagh
Copy link
Member

@sneridagh sneridagh commented Feb 27, 2019

@nileshgulia1 Can you please do a couple of changes?

We cannot ship it just like this:

screenshot 2019-02-27 at 15 43 34

We need to make it a bit more human. Plone does this, which is not also super fancy, but it is something...

screenshot 2019-02-27 at 15 46 11

We need to use momentjs (we already have it in Volto) to make it look nicer. Please can you take a look at it?

Also, I want you to add a proper EventView for the Event content type (add it to the default config),

export const contentTypesViews = {
So we are not reusing the DocumentView.

@sneridagh
Copy link
Member

@sneridagh sneridagh commented Feb 27, 2019

@nileshgulia1 I know that there is no mockup for it, but can you please also make it show all the fields that are in the Event content type? Like location, etc... Take a look into the Plone template to know which ones are shown (the ones in the main tab, except the metadata (change etc..) )

@tisto
Copy link
Member

@tisto tisto commented Mar 19, 2019

@nileshgulia1 do you plan to continue to work on this?

@nileshgulia1
Copy link
Member Author

@nileshgulia1 nileshgulia1 commented Mar 19, 2019

@pnicolli
Copy link
Contributor

@pnicolli pnicolli commented Jun 23, 2019

Continued work on this during Beethoven Sprint. Up to now, we have a view that shows all fields except recurrence. Also, i18n and some tests are needed.

@pnicolli
Copy link
Contributor

@pnicolli pnicolli commented Jun 23, 2019

Added more tests. Will have a look at recurrence in the next few hours, at least for the View.

@tisto
Copy link
Member

@tisto tisto commented Sep 20, 2019

@pnicolli what's the status of this PR? Still WIP or something we should review/look into?

@nileshgulia1
Copy link
Member Author

@nileshgulia1 nileshgulia1 commented Sep 20, 2019

@tisto I started to look into it where @pnicolli left-off.
Just started implementing the recurrence for the View.
Not sure How it should look for the Edit though .

@pnicolli
Copy link
Contributor

@pnicolli pnicolli commented Sep 23, 2019

@tisto Unfortunately I don't remember the precise state of the PR but I think it was still waiting for recurrence handling. The conf took most of my energy in the last weeks, we are releasing the final version of the site (the one with the schedule!) in the next few days, I would have some time to work on other things after that, but it's completely ok if @nileshgulia1 goes on from where I left off, I have other PRs I can work on :)

Nilesh, if you find anything that I wrote that you want to discuss, feel free to contact me or comment the code directly here.

@nileshgulia1
Copy link
Member Author

@nileshgulia1 nileshgulia1 commented Nov 6, 2019

@pnicolli Hi. Thanks for quick fixes. I'm looking forward to add more conditionals for event occurring biweekly, 'on specific days' and 'repeating events' etc.
Expect this to be finished (at least covering major of the conditionals) by this weekend.

@nileshgulia1
Copy link
Member Author

@nileshgulia1 nileshgulia1 commented Dec 20, 2019

@pnicolli I think the basic functionality for recurrence is implemented now. Can you please do a quick sweep review and let me know of any changes?

Copy link
Contributor

@pnicolli pnicolli left a comment

@nileshgulia1 I'm testing the recurrence. I'll let you know asap, but I have a first suggestion. I see you have recently reinstalled react-helmet, but that was recently removed because unmaintained (see https://docs.voltocms.com/upgrade-guide/#forked-helmet-into-volto-core). I think you just need to change one import statement and you can remove that again.

I noticed you implemented the parsing of the rrule string yourself. Have you considered using a js library for that? For example this one: https://github.com/jakubroztocil/rrule
What do you think @sneridagh ?

@nileshgulia1
Copy link
Member Author

@nileshgulia1 nileshgulia1 commented Jan 16, 2020

@pnicolli Thanks for reviewing. I didn't noticed any such libraries exist for parsing of recurrence rules. But yes using rrule.js make sense and reduces lot of efforts.

@sneridagh
Copy link
Member

@sneridagh sneridagh commented Feb 6, 2020

@pnicolli yeah better stick to known libraries that already focus in the problem and solves it.

pnicolli
pnicolli previously approved these changes Feb 10, 2020
@pnicolli pnicolli dismissed their stale review Feb 10, 2020

Obsolete

Copy link
Member

@sneridagh sneridagh left a comment

@nileshgulia1 @pnicolli The recurrence widget for the form is not included in this PR, right? So we still have to wait for it to setup recurrence, is that correct? Any plans/updates for it?

Also, the EventWhen.jsx has two components, I'm ok with that if both are super related, but the name can be improved a bit maybe. What do you think?

@pnicolli
Copy link
Contributor

@pnicolli pnicolli commented Feb 14, 2020

@sneridagh Exactly, the widget is not part of this PR, this one just shows the recurrence info coming from Plone. @giuliaghisini is developing the widget in a separate branch. Would you prefer to have a Draft PR about that in order to see the state of the development?

Regarding this PR: Yeah, the reason why I kept them together is that they are related and are based on the same date formatting function. I will commit new names in a few minutes.
Also, one other minor reason why I kept them together is that the View folder is beginning to be quiet bloated with files imho, so we could maybe think of some different structure for v5...

@sneridagh
Copy link
Member

@sneridagh sneridagh commented Feb 14, 2020

@pnicoli It’s fine! No draft PR required at all! Thanks for the info!

@sneridagh sneridagh merged commit 27fe814 into master Feb 14, 2020
6 checks passed
@sneridagh sneridagh deleted the event_date-view branch Feb 14, 2020
sneridagh added a commit that referenced this issue Feb 14, 2020
* master:
  Fix event date view (#643)
  updated italian translations (#1176)
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

5 participants