-
Notifications
You must be signed in to change notification settings - Fork 34
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
feat(trip-form): Advanced Mode Settings Panel #749
base: master
Are you sure you want to change the base?
feat(trip-form): Advanced Mode Settings Panel #749
Conversation
amy-corson-ibigroup
commented
Jun 6, 2024
Note removing the old components is a breaking change |
I tried to generate an alpha build but it didn't work :/ |
c62c162
to
b1d9825
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's great, just a few notes for you
modeSettingValues: {} | ||
}; | ||
|
||
function pipe<T>(...fns: Array<(arg: T) => T>) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind moving this function into core utils? We are using it in a lot of places
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if this feels irrelevant to this PR we can do it later
import { useIntl } from "react-intl"; | ||
import SubSettingsPane from "../SubSettingsPane"; | ||
import generateModeButtonLabel from "../i18n"; | ||
import { invisibleCss } from ".."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there should be a an empty line between the import groups here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not getting any errors or warnings here, but it looks like there should be right?
packages/trip-form/src/MetroModeSelector/AdvancedModeSubsettingsContainer.tsx
Show resolved
Hide resolved
version "2.3.1" | ||
resolved "https://registry.yarnpkg.com/classnames/-/classnames-2.3.1.tgz#dfcfa3891e306ec1dad105d0e88f4417b8535e8e" | ||
integrity sha512-OlQdbZ7gLfGarSqxesMesDa5uz7KFbID8Kpq/SxIoNGDqY8lSYs0D+hhtBXhcdB3rcbXArFr7vlHheLk1voeNA== | ||
|
||
classnames@^2.3.1: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm, do you know why we got another version of classnames?
also update snapshots |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
amazing!
import { useIntl } from "react-intl"; | ||
import SubSettingsPane from "../SubSettingsPane"; | ||
import generateModeButtonLabel from "../i18n"; | ||
import { invisibleCss } from ".."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not getting any errors or warnings here, but it looks like there should be right?
@@ -31,6 +33,8 @@ export { | |||
GeneralSettingsPanel, | |||
getBannedRoutesFromSubmodes, | |||
MetroModeSelector, | |||
AdvancedModeSettingsButton, | |||
AdvancedModeSubsettingsContainer, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sort components
background-color: ${props => props.accentColor}; | ||
color: #fff; | ||
border-bottom-left-radius: ${props => props.subsettings && 0} !important; | ||
border-bottom-right-radius: ${props => props.subsettings && 0} !important; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sort css
svg { | ||
height: 26px; | ||
width: 26px; | ||
fill: ${props => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sort css
BREAKING CHANGE: removes mode subsettings panel Co-authored-by: Daniel Heppner <daniel.heppner@ibigroup.com> Co-authored-by: Amy Corson <115499534+amy-corson-ibigroup@users.noreply.github.com>