-
Notifications
You must be signed in to change notification settings - Fork 78
Conversation
Generated by 🚫 dangerJS |
bebc9c3
to
b36108b
Compare
OK, this is now good to go - you can see the components in "Consignments: Style" - they have no interactions |
@@ -9,7 +9,7 @@ | |||
}, | |||
"eslint.enable": false, | |||
"flow.enabled": false, | |||
|
|||
"editor.formatOnSave": true, |
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.
@mennenia @sarahscott @alloy @l2succes - this turns on prettier by default when you press save
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.
as a compulsive saver (i occasionally try to cmd-s
in real life), does this noticeably slow down the editor or hang?
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.
Nope, I'm a compulsive saver too, all these things are done async too
There is an option to run on every newline instead, but I felt that was a bit overkill
import Search from "../components/artist-search-results" | ||
import BottomAlignedButton, { BottomAlignedProps } from "../components/bottom-aligned-button" | ||
import Search from "../components/artist_search_results" | ||
import BottomAlignedButton, { BottomAlignedProps } from "../components/bottom_aligned_button" |
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.
bunch of renames, they're in a folder called consignments already, so no need for extra context
|
||
import { storiesOf } from "@storybook/react-native" | ||
import * as React from "react" | ||
import { View } from "react-native" | ||
|
||
import * as bottomAlignedButton from "./consignments-bottom-aligned.story" |
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.
Moved all of my kebab named files to lowercase, this is mainly to be consistent with code that came before me - we should consider adding a danger rule for this ( thinking of the same for Jest too jestjs/jest#3771 )
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.
Yea, I'm down for lowercase and snake case for files, def.
|
||
</g> | ||
|
||
</svg> |
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.
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.
Dude, awesome! 👏
Got a screenshot/design mockup of what it should look like for real?
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.
Yeah, it's this:
👯♂️
For now I think it's better for bigger things like #603 than this, but I wanted to get it in
Maxim already has a PR to review, @mzikherman doesn't |
import * as renderer from "react-test-renderer" | ||
import BooleanButton from "../boolean_button" | ||
|
||
it("looks good as an svg", () => { |
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.
kind of an odd test description? it can still be totally wrong but still look good!
@@ -0,0 +1,94 @@ | |||
import * as React from "react" |
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.
😎
Feel like the name isn't amazing, maybe 'Toggle' or something is better?
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 seems like the way a consumer might want to use this component, is to basically provide the left/right text. So will this component be handling the 'toggle'?
ie- should this component do the UI toggling, and then call the onPress
the client passed in (since presumably that might trigger something)?
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 was thinking that the onPress
would end up changing a parent's state that would eventually trickle down into the selected
state, so handling the transition in the component didn't feel right. If it was doing animation, I'd definitely agree though. Then it might be a onChange kind of affair.
WRT name: agreed
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.
Him, maybe an onchange is just better overall, it can provide the new state then
Adds a button for indicating boolean states inside forms
This is also the first PR with jest-snapshots-svg, and renames consignment files from
kebab-case
tosnake_case