-
-
Notifications
You must be signed in to change notification settings - Fork 159
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
feat: ondevice addon controls v1 [6.0] #182
Conversation
c72227a
to
7100708
Compare
@lauriharpf hey I was looking to see whats left to do here in order to merge and I guess it's not quite ready. However I think that there is a way to split some of this work. if you see my commit 2bd4f83 I've started re-doing the old knob components to work with controls, if you want to give it a try then pick one of the control types to add back, boolean is probably an easy one to start with. Also currently the examples are a bit useless so at some point it would be good to improve those, though I think the functionality is the first priority. |
Thanks 👍. Gave this a shot at #195 , please take a peek and see if it is in the right direction. This area isn't too familiar for me, so included some questions. I'll need to tackle some other stuff for a week or so, which might make me a bit hard to reach. Should be back in action on the 12th of July or so. It might be possible for me to squeeze out another quick PR today or tomorrow, e.g. for the |
@lauriharpf thanks again for your help, I really appreciate it. I will test your PR soon, if you need anything reach me here or via discord 🙂 . |
Shared where I ended up in #197 ; not much progress unfortunately. Feel free to pick it up or throw it away & just implement it 🙂 . |
|
||
export const Switch = ({ on }: Props) => ( | ||
<View style={[styles.circle, on ? styles.on : styles.off]}> | ||
<Text style={!on && styles.offText}>{on ? 'on' : 'off'}</Text> |
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 behaves a bit weird for me on the Android emulator:
- When I initially open it, everything looks fine:
- If I then go to "Addons -> Controls" and toggle it off, it still renders fine:
- If I again go to "Addons -> Controls" and toggle it back on, the rendering becomes wonky:
This doesn't happen on iOS. Explicitly setting the color for both off & on seems to fix this:
const styles = StyleSheet.create({
...
onText: { color: 'black' },
offText: { color: 'white' },
});
...
<Text style={on ? styles.onText : styles.offText}>{on ? 'on' : 'off'}</Text>
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.
Oh I guess there isn't a default color on Android then. I'll change this. Thanks!
- Fix eslint issues and reduce the number of items in .eslintignore. See https://stackoverflow.com/a/32899667/843984 for the "\033c" -> "\x1Bc" -change in bootstrap.js - Change the examples/native/.storybook/main.js regex to work with the ControlExamples structure
import React from 'react'; | ||
import { View } from 'react-native'; | ||
|
||
// const usePrevious = (children) => { |
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.
Could these commented-out portions be removed? 🤔
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'll remove this.
@@ -0,0 +1,43 @@ | |||
/* eslint no-underscore-dangle: 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.
Trivial, but yarn lint
seems to pass even after removing 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.
ok I can remove 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.
Phew! What an effort, well done! 👍 🚀 🥇
Tried to look at everything and also tested the controls on both platforms. IMO, we could merge this now and then tackle the remaining tasks in separate PRs. Let's try to split the work a bit today and look at how we could get the first alpha out.
Again, thank you 🙏 for this huge step towards v6! 💎
…n components. Omissions: * ondevice-controls/src/GroupTabs.tsx: Not refactored, as the component is currently unused. Let's take it into use before refactoring or remove it (see storybookjs#182 (comment)) * ondevice-controls/src/components/color-picker/*: Let's try to get the fixes PR'd and merged to the original library (so we can remove this code) or try to find a color picker that doesn't need custom modifications
…n components. Omissions: * ondevice-controls/src/GroupTabs.tsx: Not refactored, as the component is currently unused. Let's take it into use before refactoring or remove it (see storybookjs#182 (comment)) * ondevice-controls/src/components/color-picker/*: Let's try to get the fixes PR'd and merged to the original library (so we can remove this code) or try to find a color picker that doesn't need custom modifications
Issue: #177
What I did
I've started converting knobs into controls.
Things missing
fix hooks context or make a custom one, hooks context from storybook/api hooks use the window objectcurrently the state only updates when until the preview is refocusedhacky fix added"reset" doesn't reset the control state correctlyadd back in all the other typeswill be continued in future PR'sbig refactor neededpartially done, will also be part of future work.After this PR is merged this issue #181 should be picked up to improve the re-usability of these components.
How to test
testing steps to come.