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.

Sign up for GitHub

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

Jump to bottom

"Back to calendar" link in event page when opened in new tab/window #306

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

"Back to calendar" link in event page when opened in new tab/window #306

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
972a5d4
"Back to calendar" link in event page when opened in new tab/window
graymur Oct 24, 2017
e21c2e1
replaces Object.assign with object spread
graymur Oct 26, 2017
b6211f6
Refactored settings calendat's id on event
graymur Oct 26, 2017
54e5f4f
Merge branch 'master' into dev.item-page-back-link
Restuta Oct 26, 2017
449168a
getCalendarSlug util function
graymur Oct 27, 2017
2e3725b
Styling Back button
graymur Nov 14, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions src/client/calendar/events/event-details/EventDetails.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,8 @@ class EventDetails extends Component {
promoterInfo,
} = this.props.event

const { calendar } = this.props
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about destructuring this all together below with movedToEvent?


// TODO: migrate old events to have flyer section and not just "flyerUrl" property
const { movedToEvent } = this.props

Expand Down Expand Up @@ -312,6 +314,7 @@ class EventDetails extends Component {
: (
// eslint-disable-next-line
<div className='EventDetails-container'>
<Link className="back-to-calendar" to={`/calendars/${calendar.slug}?past=visible`}>Back to {calendar.name}</Link>
{eventDetailsComponent}
</div>
)
Expand All @@ -321,14 +324,15 @@ class EventDetails extends Component {

EventDetails.propTypes = {
event: PropTypes.object.isRequired,
calendar: PropTypes.object,
// if event status is "Moved" this would contain event it's moved to
movedToEvent: PropTypes.object,
}

export { EventDetails }

import { connect } from 'react-redux'
import { getEvent } from 'shared/reducers/reducer.js'
import { getEvent, getCalendar } from 'shared/reducers/reducer.js'
import { replaceRoutedModal } from 'shared/actions/actions.js'

export default connect(
Expand All @@ -337,13 +341,14 @@ export default connect(
// calendar: getCalendar()

const event = getEvent(state, ownProps.params.eventId)
const calendar = getCalendar(state, {calendarId: event.calendarId})
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@graymur yeah, I know, it just became apparent here. I am fine with doing it in another PR.

let movedToEvent

if (event.status === Statuses.moved && event.movedToEventId) {
movedToEvent = getEvent(state, event.movedToEventId)
}

return { event, movedToEvent }
return { event, movedToEvent, calendar }
},
(dispatch, ownProps) => ({
replaceRoutedModal: (path) => dispatch(replaceRoutedModal({path, hasPadding: false}))
Expand Down
7 changes: 7 additions & 0 deletions src/client/calendar/events/event-details/EventDetails.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,13 @@
width: 100%;
max-width: 100%;
}

.back-to-calendar {
position: absolute;
z-index: 1;
left: 4rem;
top: 1rem;
}
}

.EventDetails {
Expand Down
16 changes: 12 additions & 4 deletions src/shared/reducers/reducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ const toByIdMap = objects => objects.reduce((map, x) => {
return map
}, {})

const setEventsCalendarId = calendarId => event => Object.assign({}, event, {calendarId})
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's babel transpiled, so you can use ...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


const toArrayOfIds = objects => objects.map(x => x.id)

const initialState = {
Expand Down Expand Up @@ -60,16 +62,17 @@ const initialState = {
},

events: toByIdMap(
norcalMtb2016Events
.concat(ncnca2016Events)
.concat(ncnca2017Events)
.concat(usac2017Events)
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')))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest using calendarId as it's really clear and maybe use secondaryCalendarsIds for others. But it's not critical.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@graymur sounds good.

),

//calenars map by id
calendars: {
['cal-norcal-mtb-2016']: {
id: 'cal-norcal-mtb-2016',
slug: 'norcal-mtb',
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Restuta maybe create a helper function getCalendarsSlug(calendar) and use it everywhere

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that makes sense.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

year: 2016,
name: '2016 NorCal MTB Calendar',
highlight: {
Expand All @@ -82,6 +85,7 @@ const initialState = {
},
['cal-ncnca-2017-draft']: {
id: 'cal-ncnca-2017-draft',
slug: 'ncnca-2017',
year: 2017,
name: '2017 NCNCA Calendar',
// highlight: {
Expand All @@ -97,6 +101,7 @@ const initialState = {
},
['cal-ncnca-2018-draft']: {
id: 'cal-ncnca-2018-draft',
slug: 'ncnca-2017',
year: 2018,
name: '2018 NCNCA Calendar',
// highlight: {
Expand All @@ -113,6 +118,7 @@ const initialState = {
},
['cal-ncnca-2016']: {
id: 'cal-ncnca-2016',
slug: 'ncnca-2016',
year: 2016,
name: '2016 NCNCA Calendar',
// highlight: {
Expand All @@ -127,6 +133,7 @@ const initialState = {
},
['cal-ncnca-2017']: {
id: 'cal-ncnca-2017',
slug: 'ncnca-2017',
year: 2017,
name: '2017 NCNCA Calendar',
// highlight: {
Expand All @@ -141,6 +148,7 @@ const initialState = {
},
['cal-usac-2017']: {
id: 'cal-usac-2017',
slug: 'usac-2017',
year: 2017,
name: '2017 USA Cycling Calendar',
// highlight: {
Expand Down