Use dynamic contents via React context

[evansiroky]

This PR depends on #201.

In response to #201 (comment), this PR refactors the app to avoid needless prop-drilling of custom components sent in by implementing applications by utilizing the React context as needed.

Although this PR doesn't yet have any commits that indicate a breaking change, there are changes that could be considered breaking. The itineraryClass, itineraryFooter, LegIcon and ModeIcon are now all supposed to be added to a components property in the ResponsiveWebapp instead of in individual components. This also introduces the assumption that implementers of otp-react-redux will use the ResponsiveWebapp or directly import the ComponentContext and wrap components in a Provider in their app.

[landonreed]

My primary comment (stated previously on a call, I think) is that perhaps we should allow users of this library to pass in LegIcon, ModeIcon or ItinType as a prop instead of context. And that we should perhaps not change the itineraryFooter name to avoid making this a breaking change.

[binh-dam-ibigroup]

Conditional approval on mentioning ItineraryFooter in example.js. This PR begs the question on whether state.otp.config should be a context too, but I definitely agree with the approach.

[binh-dam-ibigroup]

One of my first reactions as I am reading about React.Context is that all of state.otp.config could be moved to the context (it is read once a few lines above and set once for all to global state). What are your thoughts on that?

[evansiroky]

I was also thinking that, but it'd be a much larger change to instead include that in the context instead of the redux store. Also, perhaps some parts of the config might be changeable, for example if we ever implement a webapp with multiple languages, maybe it could have a config setting of the language that could change in the app.

[evansiroky]

The more I think about it tho, maybe it would be good to consider the config being a global context thing that is assumed to not change. It could be useful to even explore adding some kind of context provider to otp-ui packages which would reduce the amount of prop-drilling and simplify the inclusion of config items throughout those components.

[binh-dam-ibigroup]

Inconsistent spacing between braces.

[evansiroky]

Fixed in 91bdbb5.

[binh-dam-ibigroup]

I think ItineraryFooter should be mentioned here, at least as a commented-out attribute, so implementers know that it is possible to provide a footer if they so wish.

[evansiroky]

Improved above comment to note ItineraryFooter in 91bdbb5.

[evansiroky]

@landonreed, in response to your comment

My primary comment (stated previously on a call, I think) is that perhaps we should allow users of this library to pass in LegIcon, ModeIcon or ItinType as a prop instead of context. And that we should perhaps not change the itineraryFooter name to avoid making this a breaking change.

The more I think of this, the more I'm thinking this is kind of a take-it-or-leave-it PR. If we want to keep this behavior for any of the *Panel components, then this would re-introduce the prop-drilling problem. If it's for the final components that use the context, it may not be possible for end-users to configure these properly without reintroducing prop-drilling since some components like RouteRow in RouteViewer aren't exported directly.

However, it is possible for an end-user to still use components directly without being inside the ResponsiveWebapp component. I have added some comments about how to do this in 91bdbb5. In all reality, it seems unlikely that any end-user would import individual components directly given how tightly tied to redux the components in this repo are.

[evansiroky]

Ready for review again, and I'm reassigning @binh-dam-ibigroup to consider #274 (comment).

[binh-dam-ibigroup]

LGTM.

[landonreed]

@evansiroky, your comment about providing the config as context to otp-ui components made me think of this: if components like the StopViewer (or others that make use of context) were to migrate to otp-ui, how would we handle providing them with the context? Maybe this is just a bridge we cross when we get there, but just wanted to get the thought down.

[evansiroky]

@evansiroky, your comment about providing the config as context to otp-ui components made me think of this: if components like the StopViewer (or others that make use of context) were to migrate to otp-ui, how would we handle providing them with the context? Maybe this is just a bridge we cross when we get there, but just wanted to get the thought down.

I'm changing my mind again and thinking that maybe slots is actually better for otp-ui after all. Currently, otp-rr already sends appropriate components defined in the context like this:

otp-react-redux/lib/components/form/connected-settings-selector-panel.js

Lines 13 to 41 in 91bdbb5

	class ConnectedSettingsSelectorPanel extends Component {
	static contextType = ComponentContext
	render () {
	const {
	config,
	query,
	setQueryParam,
	showUserSettings
	} = this.props
	const { ModeIcon } = this.context
	return (
	<div className='settings-selector-panel'>
	<div className='modes-panel'>
	{showUserSettings && <UserTripSettings />}
	<StyledSettingsSelectorPanel
	ModeIcon={ModeIcon}
	queryParams={query}
	supportedModes={config.modes}
	supportedCompanies={config.companies}
	onQueryParamChange={setQueryParam}
	/>
	</div>
	</div>
	)
	}
	}

Since each otp-ui package kind of stands by itself, it seems like it could be more work and unfriendly to first-time users to have to wrap it in a context provider in order for the package to work. Perhaps within each package it could make sense to use the context API to reduce prop-drilling inside the components in the package.

[github-actions]

🎉 This PR is included in version 2.0.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀