-
Notifications
You must be signed in to change notification settings - Fork 5
added clear button #59
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
Conversation
src/components/FuncCard.js
Outdated
@@ -21,6 +21,7 @@ class FuncCard extends Component { | |||
this.handleToggleCode = this.handleToggleCode.bind(this); | |||
this.handleParamsChange = this.handleParamsChange.bind(this); | |||
this.handleRunCode = this.handleRunCode.bind(this); | |||
this.handleClearCode = this.handleClearCode.bind(this); |
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.
Maybe handleClearParams
instead of handleClearCode
?
src/components/FuncCard.js
Outdated
<Highlight lang="js" value={callCode} /> | ||
|
||
<div className="results-div"> | ||
<Button floating className='grey darken-3 small-btn' waves='light' icon='clear' onClick={this.handleClearCode}>Clear</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.
Disabling the button when there are no params might be good.
disabled={params.length === 0}
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 like that - and if there is not a value in result
Yea it didn't look good adjacent to the params bar so that's why I put some opacity. Mentioned previously that we can move button wherever. I don't have any strong feels about where it lives. |
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 go forward with this for now. We can improve later 😄
#60 - I bet this will fix the mobile issue: there was an option to render as "children" in the docs. |
<Carousel> | ||
{funcCards} | ||
</Carousel> | ||
<Carousel children={funcCards} /> |
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 doesn't change anything 😞 It doesn't matter how you set the children property, it will end up as props.children in Carousel component. The final result is the same.
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 doesn't work even if I replace everything in App with example code from https://react-materialize.github.io/#/carousel.
#50
Clear button styled and working. We can decide to move it wherever or leave it, but wanted to get this out while I had the chance. I also extended the width of the results component to try to squeeze a little extra room for text.