DT-6175 part 4 #4937
Merged
DT-6175 part 4 #4937
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
vesameskanen
commented
Jan 26, 2024
•
vesameskanen
commented
- Refactor Itinerary page jsx rendering by using shared code blocks, don't repeat same code again and again
- Remove duplicate query
- Shared wrapper for detail view
- Correct spinners while loading
- Correct notifiers and error messages
- Refactor planParamUtil and settings management
- Simplify
- Remove annoying ticket name comments from code
- remove if-else maze which exceeds human comprehension - put shered jsx into variables - move detail view tab wrapping to new component ItineraryTabs - remove redundant MobileItineraryWrapper
We want to keep code clean and readable, and use git for documenting change history.
All building blocks are now shared between mobile and desktop. Desktop view composition is defined in 24 code lines (was 243 lines).
Also, tune spinning for alt itinerary view
- remove strange duplicate and incorrect notifier - don't pass existing context fields as props - cleanup components
- add basic loading state because root props.loading sets lazily in refetch - define driving and biking props correctly for notifiers
optionsome
reviewed
Jan 29, 2024
app/component/ItineraryPage.js
Outdated
Comment on lines
262
to
263
| // show 6 bike + transit itineraries, preferably 3 of both kind. | ||
| // If there is not enough of a kind, take more from the other kind |
There was a problem hiding this comment.
Strange. Must be some git merge thing. Restored now.
app/component/ItineraryPage.js
Outdated
Comment on lines
475
to
468
| this.setState({ | ||
| loadingMore: reversed ? spinnerPosition.bottom : spinnerPosition.top, | ||
| }); | ||
| this.showScreenreaderLoadingAlert(); | ||
| this.setState({ loadingMore: reversed ? 'bottom' : 'top' }); |
There was a problem hiding this comment.
Why do we go back to hardcoding strings here?
There was a problem hiding this comment.
Same as above. A bit worrying. Restored.
app/component/ItineraryPageUtils.js
Outdated
| @@ -358,6 +360,8 @@ export function checkDayNight(iconId, timem, lat, lon) { | |||
| return iconId; | |||
| } | |||
|
|
|||
| const streetLegModes = ['WALK', 'BICYCLE', 'CAR', 'SCOOTER']; | |||
There was a problem hiding this comment.
Spelling of these static helper variables could be STREET_LEG_MODES but not that important.
No need to use a function returning function returning function, as amusing it is to maximize complexity. Improve names and remove duplicate parameter.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.