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.

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

Add scooter stop fallback message #767

Open
wants to merge 3 commits into
base: next
Choose a base branch
from

Conversation

Beck-berry
Copy link

@Beck-berry Beck-berry commented Sep 11, 2021

Proposed Changes

  • extended the bike rental station query with the networks property
  • added condition: display a fallback message based on station.networks if station.name is not given or if the network is tier-reutlingen

image

Pull Request Check List

  • A reasonable set of unit tests is included
  • Console does not show new warnings/errors
  • Changes are documented or they are self explanatory
  • This pull request does not have any merge conflicts
  • All existing tests pass in CI build
  • Code coverage does not decrease (unless measured incorrectly)

Review

  • Read and verify the code changes
  • Test the functionality by running the UI locally with all popular browsers available in your platform
  • Check that the implementation matches the design, when such one is defined in a Jira issue
  • Merge the pull request

Closes #760

@Beck-berry Beck-berry self-assigned this Sep 11, 2021
@Beck-berry
Copy link
Author

Beck-berry commented Sep 24, 2021

  • Use network in the fallback message id.

@Beck-berry
Copy link
Author

@hbruch Could you please define a condition for exactly when should we use the fallback message? The easiest case (which the original ticket mentioned) is when there is no name given for the station -- but what should I examine when there is a name but it is not comprehensible for the user (basically the ID)? I just found out that this is possible, too.
My first thought was to check the length of the string, and if it is longer than to fit in the popup, we could use the default message. What do you think?

@hbruch
Copy link

hbruch commented Sep 26, 2021

Why not only show " station". Maybe we have to decide per network, if station.name is useful to display

@Beck-berry Beck-berry requested a review from hbruch September 26, 2021 21:16
<div className="selected-stop-header">{stop.name}</div>
<div className="selected-stop-header">
{!stop.name || stop.networks[0] === 'tier_REUTLINGEN' ? (
<FormattedMessage
Copy link

Choose a reason for hiding this comment

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

We should not use network specific code here. Is there a way to deduce the vehicleType from the stop and use a vehicleType specific message?

Copy link
Author

Choose a reason for hiding this comment

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

I used this solution because originally the popup should show the name of the actual stop - but in case of this network, the name is an incomprehensible number code. If we'd use any other condition, this process will change. Should we refactor this anyway and do not use the name (not at any stop popup) but display a message based on the vehicle type?

app/component/map/StopPageMap.js Outdated Show resolved Hide resolved
@@ -19,6 +19,10 @@ BikeRentalStationPageMapContainer.propTypes = {
lat: PropTypes.number.isRequired,
lon: PropTypes.number.isRequired,
name: PropTypes.string,
networks: PropTypes.oneOfType([
PropTypes.string,
Copy link

Choose a reason for hiding this comment

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

When will networks be string, when array?

Copy link
Author

Choose a reason for hiding this comment

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

It's always a string array. When it has only one element, it can be handled as a string.

@Beck-berry Beck-berry force-pushed the feat/scooter_stop_fallback_msg branch from ae2cc89 to 4976661 Compare October 12, 2021 09:02
@Beck-berry Beck-berry requested a review from hbruch October 12, 2021 09:04
@Beck-berry
Copy link
Author

Depending on #774. Needs refactoring after the merge of that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use fallback for bike sharing popup when name is empty
2 participants