"Back to calendar" link in event page when opened in new tab/window#306
"Back to calendar" link in event page when opened in new tab/window#306
Conversation
graymur
commented
Oct 24, 2017
graymur
commented
- Now calendar's id is added to an event, so it's possible to find calendar by an event
- Calendars objects have "slug" field to simplify URLs creation
|
Do you have a screenshot? @graymur |
Restuta
left a comment
Restuta
left a comment
There was a problem hiding this comment.
Thanks a lot for this PR, left few comments and haven't actually tested UI yet.
| calendars: { | ||
| ['cal-norcal-mtb-2016']: { | ||
| id: 'cal-norcal-mtb-2016', | ||
| slug: 'norcal-mtb', |
There was a problem hiding this comment.
Slug is actually a calculated property, you can see how it's done here https://github.com/Restuta/rcn.io/blob/master/src/client/calendar/utils/get-calendar-id.js in reverse.
Convention is slug = calendarId.replace('cal-', ''). So if you go on live
https://rcn.io/calendars/usac-2017 (is canonical url)
https://rcn.io/calendars/cal-usac-2017 (also available, but we don't link to it anywhere and ideally should redirect to non-cal version)
So part of calendar id after cal- serves as slug, therefore you don't ave to store it.
norcal-mtb breaks this rule and I would suggest renaming it and setting redirect in router from old route (norcal-mtb) to norcal-mtb-2016.
There was a problem hiding this comment.
@Restuta maybe create a helper function getCalendarsSlug(calendar) and use it everywhere
src/shared/reducers/reducer.js
Outdated
| return map | ||
| }, {}) | ||
|
|
||
| const setEventsCalendarId = calendarId => event => Object.assign({}, event, {calendarId}) |
There was a problem hiding this comment.
it's babel transpiled, so you can use ...
src/shared/reducers/reducer.js
Outdated
| norcalMtb2016Events.map(setEventsCalendarId('cal-norcal-mtb-2016')) | ||
| .concat(ncnca2016Events.map(setEventsCalendarId('cal-ncnca-2016'))) | ||
| .concat(ncnca2017Events.map(setEventsCalendarId('cal-ncnca-2017'))) | ||
| .concat(usac2017Events.map(setEventsCalendarId('cal-usac-2017'))) |
There was a problem hiding this comment.
I think this line gets confusing with this amount of manipulations. Let's refactor to:
const _ = require('lodash')
//..
const setCalendarId = calendarId => event => ({...event, calendarId })
const toByIdMap = _.partialRight(_.keyBy, 'id')
const result = toByIdMap(
_.concat(
norcalMtb2016Events.map(setCalendarId('cal-norcal-mtb-2016')),
ncnca2016Events.map(setCalendarId('cal-ncnca-2016')),
//...
)
)Notice shorter name of the function setCalendarId and refactoring of toByIdMap to use lodash
Also, I forgot to mention that I plan to set a relationship as many-to-many. So event can belong to multiple calendars.
Use-case would be you would want to pick events from multiple calendars and create your own or combine multiple calendars into own calendar. But every even also should point to it's "primaryCalendar" (not sure if "primary" is a good name) where it was created originally.
With that I think we should name this property as "primaryCalendarId" since later we would have "calendarIds" and having singular "calendarId" along with plural "calendarIds" would be confusing.
Open for naming ideas.
There was a problem hiding this comment.
I'd suggest using calendarId as it's really clear and maybe use secondaryCalendarsIds for others. But it's not critical.
| promoterInfo, | ||
| } = this.props.event | ||
|
|
||
| const { calendar } = this.props |
There was a problem hiding this comment.
How about destructuring this all together below with movedToEvent?
| // calendar: getCalendar() | ||
|
|
||
| const event = getEvent(state, ownProps.params.eventId) | ||
| const calendar = getCalendar(state, {calendarId: event.calendarId}) |
There was a problem hiding this comment.
I think we should be consistent with simple selectors like that and either use named arguments or don't. Above selector doesn't use those since it's pretty self-explanatory when you see like like this:
const calendar = getCalendar(state, event.calendarId)
Thoughts?
There was a problem hiding this comment.
Yes, but getCalendar selector was alredy their like that, so I reused without changing it's params. I can refactor this, but maybe it will be better to do it in separate PR?
There was a problem hiding this comment.
@graymur yeah, I know, it just became apparent here. I am fine with doing it in another PR.
|
Here's a screenshot: |
|
@graymur thanks, that would require some revision on my side. I will contribute to this PR to make it the way I imagined, sorry for not providing designs. |
| @@ -0,0 +1,3 @@ | |||
| const getCalendarSlug = calendar => calendar.id.replace('cal-', '') | |||
There was a problem hiding this comment.
@graymur this function doesn't require entire calendar then, since it only uses id
There was a problem hiding this comment.
What if this changes in the future? I think it's quite possible.
There was a problem hiding this comment.
Why do you think so? But if it does we will refactor.
|
Here's how I've impelemented it: |
2 participants
