Skip to content
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

Add default button and example #152

Merged
merged 12 commits into from
Jan 3, 2019

Conversation

akinsho
Copy link
Member

@akinsho akinsho commented Dec 23, 2018

default_revery_button

fixes #37

render(
() => {
let width =
Monitor.getPrimaryMonitor()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the default <Button /> styling - I think we should decouple this from the monitor size, and instead using some default padding (along with perhaps a minWidth). It'd be helpful to be able to drop a <Button /> anywhere, like in a side-pane - my concern is if we base it on 30% of the monitor size, it might not fit well in places you drop it (like if we had a 'left pane' / 'sidebar' for our examples)

Copy link
Member

@bryphe bryphe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the work on this and the feedback! It's looking really cool 👍

Had one piece of feedback regarding the default width sizing - let me know what you think.

@akinsho
Copy link
Member Author

akinsho commented Dec 27, 2018

@bryphe I've tweaked what you suggested (was more of a thing I had to test it out), I'm trying to make the example more dynamic using a stateHook to trigger a counter to show that disabled buttons do nothing and enabled buttons work but I don't think I'm using the hook correctly or else there a bug somewhere I could use some input 🙏

@OhadRau
Copy link
Collaborator

OhadRau commented Dec 28, 2018

@Akin909 any luck figuring out that bug yet? I looked through the code and couldn't find a clear issue. FWIW your use of useState looked identical to the way it's used in examples/Bin.re so I don't think that's the culprit.

@akinsho
Copy link
Member Author

akinsho commented Dec 28, 2018

@OhadRau thanks for having a look 👍 , the strange thing is that the the click event doesn't seem to work, I'm not sure if the state isn't updating, or if the onClick function isn't being called, I'm beginning to think that the event might be being blocked, or is not transmitting through properly

@OhadRau
Copy link
Collaborator

OhadRau commented Dec 28, 2018

Ah, I think I tracked it down. Looks like this is a rendering bug since printing from onMouseDown/onMouseUp works but the opacity isn't getting set. Not sure why, but the view doesn't seem to get rerendered here at all.

I thought the bug might be that setShouldRenderCallback isn't getting called, since the only current call to setShouldRenderCallback occurs when there's an animation. However, changing that doesn't cause it to re-render here for some reason... though the counter is certainly being incremented:

image

Will have to investigate this some more later

@Akin909

@akinsho
Copy link
Member Author

akinsho commented Dec 29, 2018

Thanks for such a thorough look @OhadRau, much appreciated I'm still trying to figure out how to debug stuff effectively in this environment. I'll have a look into why the render callback doesn't return true

@bryphe
Copy link
Member

bryphe commented Dec 29, 2018

Thanks @Akin909 and @OhadRau for the detailed investigation (and for logging the bug #170 ). I put together a proposal for a fix #171 (and verified it unblocks this too 👍 ).

@akinsho akinsho added the WIP label Dec 30, 2018
@akinsho akinsho changed the title [WIP] Add default button and example Add default button and example Dec 31, 2018
@akinsho
Copy link
Member Author

akinsho commented Dec 31, 2018

@bryphe thanks for fixing #170 👍, that seems to have unblocked this, any chance you can re-review when you get a chance?

Copy link
Member

@bryphe bryphe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just tried it out and it works great! Thanks for your work on this, @Akin909 ! 🎉

@akinsho akinsho removed the WIP label Jan 1, 2019
@akinsho akinsho force-pushed the feature/add_button_component branch from c6090ab to 465ea10 Compare January 2, 2019 19:19
@akinsho
Copy link
Member Author

akinsho commented Jan 3, 2019

@bryphe CI seems to be failing but I cant figure out why. The esy.lock changed for reasons I don't quite understand could be that I'm not running the right version of esy 😕 I can't restart it though and I'm not sure what to make of the error

@bryphe
Copy link
Member

bryphe commented Jan 3, 2019

Sorry about the build issues, @Akin909 ! Not related to your changes - working on fixing them in #178

@bryphe
Copy link
Member

bryphe commented Jan 3, 2019

The code changes look great! 💯 I'll bring them in and continue working on the build failure in #178 .

So nice to have a base <Button /> control now - thanks for all the work on this!

@bryphe bryphe merged commit 51860ac into revery-ui:master Jan 3, 2019
@akinsho akinsho deleted the feature/add_button_component branch January 3, 2019 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Widget: Button
3 participants