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

fix: remove dead links #450

Merged
merged 3 commits into from
Sep 8, 2021
Merged

fix: remove dead links #450

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c2e4ca8
fix: remove dead links
miles-grant-ibigroup Sep 8, 2021
930a4a7
refactor(terms-of-use-pane): optimize redux state access
miles-grant-ibigroup Sep 8, 2021
6227886
refactor: address pr feedback
miles-grant-ibigroup Sep 8, 2021
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
14 changes: 12 additions & 2 deletions lib/components/user/terms-of-use-pane.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import React from 'react'
import { connect } from 'react-redux'
import { Checkbox, ControlLabel, FormGroup } from 'react-bootstrap'

import { TERMS_OF_SERVICE_PATH, TERMS_OF_STORAGE_PATH } from '../../util/constants'
Expand All @@ -10,6 +11,7 @@ const TermsOfUsePane = ({
disableCheckTerms,
handleBlur,
handleChange,
termsOfStorageSet,
values: userData
}) => {
const {
Expand Down Expand Up @@ -46,11 +48,19 @@ const TermsOfUsePane = ({
>
{/* TODO: Implement the link */}
Optional: I consent to the Trip Planner storing my historical planned trips in order to
improve transit services in my area. <a href={`/#${TERMS_OF_STORAGE_PATH}`} target='_blank'>More info...</a>
improve transit services in my area. {termsOfStorageSet && <a href={`/#${TERMS_OF_STORAGE_PATH}`} target='_blank'>More info...</a>}
</Checkbox>
</FormGroup>
</div>
)
}
const mapStateToProps = (state, ownProps) => {
return {
termsOfStorageSet: state.otp.config.persistence?.terms_of_storage
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add a comment in example-config.yml to show this parameter works.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need a similar config variable for the terms of 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.

Have added an example in 6227886. I don't think a similar config item for terms of use makes sense since otherwise the terms of use checkbox would not refer to any terms of use to accept!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, there is an existing component in ComponentContext in which the contents for the optional Terms of Storage is defined. Maybe you can use it in this PR to check for the existence of terms of storage instead of a new config property, or combine that in another PR. 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.

I gave this some thought, having an attribute on the "dummy" components shipped within otp-rr, and then if that attribute is missing showing the link. However, I found that all our configurations ship with the dummy components anyway, so that wouldn't work unless we changed all the configurations, which I wanted to avoid.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can't we just check for components.TermsOfStorage !== null?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Given the rush I'm inclined to accept the approach, with issue #452 to deduplicate configuration.

}
}

const mapDispatchToProps = {
}
miles-grant-ibigroup marked this conversation as resolved.
Show resolved Hide resolved

export default TermsOfUsePane
export default connect(mapStateToProps, mapDispatchToProps)(TermsOfUsePane)
7 changes: 0 additions & 7 deletions lib/util/auth.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,6 @@ export const accountLinks = [
{
text: 'My account',
url: ACCOUNT_PATH
},
{
// Add a target attribute if you need the link to open in a new window, etc.
// (supports the same values as <a target=... >).
// target: '_blank',
text: 'Help',
url: '/help'
}
]

Expand Down