-
Notifications
You must be signed in to change notification settings - Fork 7
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
925 - Routing Styling Changes #554
Conversation
07b7a05
to
a39ebab
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.
A couple of comments but otherwise LGTM
<option value="all" selected> | ||
All of | ||
</option> | ||
<option value="any" disabled> |
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.
Is it worth making it more obvious its disabled until its functional? To me, it looked as though All of was in bold because its selected and Any of was a selectable option. Same for Answer/Metadata option.
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.
Fair point 👍
@@ -154,6 +155,7 @@ Resolvers.Mutation = { | |||
}, pages); | |||
|
|||
const routing = page.routing; | |||
|
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.
This file hasn't been modified and can be removed from commit
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.
idk why it's included or how to remove!
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.
Just remove the newlines on 149 and 158 and commit again.
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 good just a couple of comments.
...or/src/App/page/Routing/RoutingPage/RoutingEditor/RuleEditor/BinaryExpressionEditor/index.js
Show resolved
Hide resolved
686e1ef
to
f59e3a1
Compare
What is the context of this PR?
Updates routing UI to meet latest designs:
BinaryExpressionEditor
buttons inlineOther changes:
TabBody
component fromEditorLayout/Tabs
—replaced usage with genericPanel
component.How to review
BinaryExpressionEditor
instances