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

"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 all commits
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
Binary file modified docs/img/event-edit-1.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
10 changes: 8 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 @@ -11,6 +11,7 @@ import GoogleStaticMap from './GoogleStaticMap.jsx'
import AddressLink from './AddressLink.jsx'
import Icon from 'atoms/Icon.jsx'
import { getEventColor } from 'client/calendar/utils/event-colors.js'
import getCalendarSlug from 'client/calendar/utils/get-calendar-slug.js'
import { locationToAddressStr } from 'client/calendar/utils/location.js'
import { Statuses, EventTypes } from 'client/calendar/events/types.js'
import classnames from 'classnames'
Expand Down Expand Up @@ -104,6 +105,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 +315,7 @@ class EventDetails extends Component {
: (
// eslint-disable-next-line
<div className='EventDetails-container'>
<Link className="back-to-calendar" to={`/calendars/${getCalendarSlug(calendar)}?past=visible`}>Back to {calendar.name}</Link>
{eventDetailsComponent}
</div>
)
Expand All @@ -321,14 +325,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 +342,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
22 changes: 21 additions & 1 deletion 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 @@ -10,12 +10,32 @@
margin-right: auto;
border-radius: pxToRem(3);
@include box-shadow-soft();
border-radius: pxToRem(3);

@include phone {
width: 100%;
max-width: 100%;
}

@media (max-width: #{map-get($grid-breakpoints, "lg")}) {
margin-top: 9rem;
}

.back-to-calendar {
position: absolute;
z-index: 1;
left: 0;
top: -8.5rem;
@include box-shadow-soft();
border-radius: pxToRem(3);
padding: 1.5rem 4rem;
color: #757575;

&:before {
content: '\2190';
position: absolute;
left: 1.5rem;
}
}
}

.EventDetails {
Expand Down
3 changes: 3 additions & 0 deletions src/client/calendar/utils/get-calendar-slug.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
const getCalendarSlug = calendar => calendar.id.replace('cal-', '')
Copy link
Owner

Choose a reason for hiding this comment

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

@graymur this function doesn't require entire calendar then, since it only uses id

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What if this changes in the future? I think it's quite possible.

Copy link
Owner

Choose a reason for hiding this comment

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

Why do you think so? But if it does we will refactor.


export default getCalendarSlug
27 changes: 15 additions & 12 deletions src/shared/reducers/reducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {
} from 'client/temp/events.js'
import { createSelector } from 'reselect'
import Colors from 'client/styles/colors'

import { partialRight, concat, keyBy } from 'lodash'

/* current event flow:
=> read from file
Expand All @@ -19,13 +19,8 @@ import Colors from 'client/styles/colors'
=> selector: create event's map by date
*/

//TODO bc: set calendar ID to every event, but don't do it in this function
// it should be done at the time of creation of imported events
const toByIdMap = objects => objects.reduce((map, x) => {
map[x.id] = x
return map
}, {})

const toByIdMap = partialRight(keyBy, 'id')
const setCalendarId = calendarId => event => ({...event, calendarId})
const toArrayOfIds = objects => objects.map(x => x.id)

const initialState = {
Expand Down Expand Up @@ -60,16 +55,19 @@ const initialState = {
},

events: toByIdMap(
norcalMtb2016Events
.concat(ncnca2016Events)
.concat(ncnca2017Events)
.concat(usac2017Events)
concat(
norcalMtb2016Events.map(setCalendarId('cal-norcal-mtb-2016')),
ncnca2016Events.map(setCalendarId('cal-ncnca-2016')),
ncnca2017Events.map(setCalendarId('cal-ncnca-2017')),
usac2017Events.map(setCalendarId('cal-usac-2017')),
)
),

//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 +80,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 +96,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 +113,7 @@ const initialState = {
},
['cal-ncnca-2016']: {
id: 'cal-ncnca-2016',
slug: 'ncnca-2016',
year: 2016,
name: '2016 NCNCA Calendar',
// highlight: {
Expand All @@ -127,6 +128,7 @@ const initialState = {
},
['cal-ncnca-2017']: {
id: 'cal-ncnca-2017',
slug: 'ncnca-2017',
year: 2017,
name: '2017 NCNCA Calendar',
// highlight: {
Expand All @@ -141,6 +143,7 @@ const initialState = {
},
['cal-usac-2017']: {
id: 'cal-usac-2017',
slug: 'usac-2017',
year: 2017,
name: '2017 USA Cycling Calendar',
// highlight: {
Expand Down
21 changes: 3 additions & 18 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -952,16 +952,9 @@ [email protected]:
dependencies:
hoek "2.x.x"

[email protected]:
version "4.0.0-alpha.6"
resolved "https://registry.yarnpkg.com/bootstrap/-/bootstrap-4.0.0-alpha.6.tgz#4f54dd33ac0deac3b28407bc2df7ec608869c9c8"
dependencies:
jquery ">=1.9.1"
tether "^1.4.0"

bower@^1.7.9:
version "1.8.0"
resolved "https://registry.yarnpkg.com/bower/-/bower-1.8.0.tgz#55dbebef0ad9155382d9e9d3e497c1372345b44a"
[email protected]:
version "4.0.0-alpha.2"
resolved "https://registry.yarnpkg.com/bootstrap/-/bootstrap-4.0.0-alpha.2.tgz#1238dd5c482d2545f2be78ab0d98731eb93e50c1"

brace-expansion@^1.0.0, brace-expansion@^1.1.7:
version "1.1.7"
Expand Down Expand Up @@ -2648,10 +2641,6 @@ joi@^10.6.0:
items "2.x.x"
topo "2.x.x"

jquery@>=1.9.1:
version "3.2.1"
resolved "https://registry.yarnpkg.com/jquery/-/jquery-3.2.1.tgz#5c4d9de652af6cd0a770154a631bba12b015c787"

js-base64@^2.1.9:
version "2.1.9"
resolved "https://registry.yarnpkg.com/js-base64/-/js-base64-2.1.9.tgz#f0e80ae039a4bd654b5f281fc93f04a914a7fcce"
Expand Down Expand Up @@ -4496,10 +4485,6 @@ tar@^2.0.0, tar@^2.2.1:
fstream "^1.0.2"
inherits "2"

tether@^1.4.0:
version "1.4.0"
resolved "https://registry.yarnpkg.com/tether/-/tether-1.4.0.tgz#0f9fa171f75bf58485d8149e94799d7ae74d1c1a"

text-table@~0.2.0:
version "0.2.0"
resolved "https://registry.yarnpkg.com/text-table/-/text-table-0.2.0.tgz#7f5ee823ae805207c00af2df4a84ec3fcfa570b4"
Expand Down