-
Notifications
You must be signed in to change notification settings - Fork 53
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
Create Global Trip settings and variables #1240
Create Global Trip settings and variables #1240
Conversation
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.
looks really good!
import * as apiActions from '../../actions/api' | ||
import * as formActions from '../../actions/form' | ||
import { AppReduxState } from '../../util/state-types' | ||
import { ComponentContext } from '../../util/contexts' |
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 imports
|
||
import { blue, getBaseColor } from '../util/colors' | ||
import { Close } from '@styled-icons/fa-solid' |
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 imports
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 tried to sort these and literally couldn't get eslint to be happy, so good luck
import PageTitle from '../util/page-title' | ||
|
||
import React, { RefObject, useContext } from 'react' | ||
import styled from 'styled-components' |
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 imports
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.
A few notes but I think this looks really good! I just want to see what we do with the subheaders before approving
|
||
import { blue, getBaseColor } from '../util/colors' | ||
import { Close } from '@styled-icons/fa-solid' |
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 tried to sort these and literally couldn't get eslint to be happy, so good luck
@@ -135,15 +158,13 @@ const AdvancedSettingsPanel = ({ | |||
<Subheader> |
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.
Let's hide this header if there are no global settings.
Also: Do we need these headers? I don't think they add much value and they take up vertical space.
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 left them invisible for screenreaders since I think they're helpful context, and so some configurations could have access to them! I took them out, let me know what you think? I kinda miss them but I see what you mean.
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.
Looks great! I think it's good without the headers personally. Maybe there's a bit too much space between advanced settings and the settings though now? Looks good either way!
69d70c6
into
master-trip-form-update
Description:
PR Checklist: