Add mobile search screen for batch routing #332
Conversation
lib/components/mobile/main.js
Outdated
| // Render batch search screen if batch routing enabled. Otherwise, | ||
| // default to standard search screen. | ||
| const SearchScreen = isBatchRoutingEnabled(config) | ||
| ? BatchSearchScreen |
There was a problem hiding this comment.
This kind of goes against the idea that the implementing application is able to choose between either the DefaultMainPanel and the BatchRoutingPanel. In here, the BatchSearchScreen and all its associated dependencies are always imported even if they aren't supposed to be used. Perhaps the SearchScreen can be made into a required prop that must be set by the implementing app.
There was a problem hiding this comment.
Yea, agreed. I had this thought also. Thanks for flagging. I was considering whether SearchScreen should be a required prop or if we should have mobileView be configurable in trimet-mod-otp and have a separate file from main.js for batch routing that handles the functionality/logic from this file. Right now, there's only minor differences between the two (the search screen), but the option to define different mobileViews in trimet-mod-otp might be nice to have. Thoughts?
There was a problem hiding this comment.
After looking at this more, I'm not sure why there are even slots for desktopView and mobileView at all, especially now that we're allowing the configuration of custom components via the context API. The example.js and trimet-mod-otp files are nearly identical in how they render all this. Also, all the stuff in desktopView and mobileView is super connected to all the otp-react-redux internals that I don't think you'd ever want to define a cust mobileView and if you did, you'd just be writing a different app anyways.
I've gone ahead and refactored ResponsiveWebapp so that it does away with the desktopView and mobileView slots completely and now it relies more on the Context API for the things we did want to customize. See #337.
|
There's a lot of unused space in the date/time section.
It'd look better, especially in mobile, for this area to only go as far down as just below the last element. |
evansiroky
left a comment
evansiroky
left a comment
There was a problem hiding this comment.
See comments. Also, one of the GitHub Actions is failing.
|
@evansiroky, thanks for the comments. Quick question for you above regarding the mobile search screen. |
binh-dam-ibigroup
left a comment
binh-dam-ibigroup
left a comment
There was a problem hiding this comment.
LGTM. I'll go with @evansiroky's comments and give a pass on props ordering.
evansiroky
left a comment
evansiroky
left a comment
There was a problem hiding this comment.
See proposed changes in #337.
BREAKING CHANGE: refactored example.js, ResponsiveWebapp and associated component signatures to use components provided to component context
ResponsiveWebapp refactor
|
I've merged in #337 and also fixed the ItineraryCarousel to properly show the expanded ItineraryBody. |
Codecov Report
@@ Coverage Diff @@
## dev #332 +/- ##
========================================
- Coverage 9.78% 9.03% -0.75%
========================================
Files 115 178 +63
Lines 4078 6528 +2450
Branches 1077 1683 +606
========================================
+ Hits 399 590 +191
- Misses 3191 5100 +1909
- Partials 488 838 +350
Continue to review full report at Codecov.
|
binh-dam-ibigroup
left a comment
binh-dam-ibigroup
left a comment
There was a problem hiding this comment.
Approval stands, just one tiny item to clean up.
| import BatchPreferences from './batch-preferences' | ||
| import DateTimeModal from './date-time-modal' | ||
| import ModeButtons, {MODE_OPTIONS, StyledModeButton} from './mode-buttons' | ||
| // import UserSettings from '../form/user-settings' |
There was a problem hiding this comment.
Let's remove this import.
|
🎉 This PR is included in version 3.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This PR partially accomplishes mobile compatibility for batch routing by updating the search screen to: