Nema Stepper Element #136
Nema Stepper Element #136
Conversation
|
@AriellaE any feedback on this PR? |
|
Hi @bonnyr , thank you for creating all these steppers sizes. Can you please implement the size change feature in our original stepper motor? |
|
@AriellaE did you mean you want me to modify the original? |
|
@bonnyr yes please |
|
@AriellaE - modified stepper element as requested (removed the added nema-* variant) |
bonnyr
force-pushed
the
add-nema-stepper
branch
from
c758205 to
efe1a23
Compare
AriellaE
left a comment
AriellaE
left a comment
There was a problem hiding this comment.
Thank you.
It looks good on the functional level.
I reviewed your changes and added some comments.
In addition here are some general requests:
- please format the code with prettier ( run
npx prettier --write src/**.ts) - match colors to the original stepper so it looks metallic
unitsandvaluetexts should go in separate lines, just like the original stepper. the value should have a bigger font.
Bonus: try to make the center of rotation more accurate, so the rotator won't shift as you rotate it.
src/stepper-motor-element.ts
Outdated
|
|
||
| render() { | ||
| const { arrow } = this; | ||
| // local vars const ratio = 1; //100 / spec.frameSize; |
There was a problem hiding this comment.
done in latest commit
src/stepper-motor-element.ts
Outdated
| { name: 'B-', y: 237, x: 127, number: 4, signals: [] }, | ||
| ]; | ||
| render() { | ||
| const spec = this.nemaSpecMap[this.size]; |
There was a problem hiding this comment.
please add fallback behavior when size is not valid (e.g. use the values of size 17, or the closest size)
There was a problem hiding this comment.
isn't this what happens when specifying the property as one of a set of values?
There was a problem hiding this comment.
Storybook does validate the size, but in Wokwi users can change the size on the diagram to an invalid value and then they get an error.
src/stepper-motor-element.ts
Outdated
| xmlns="http://www.w3.org/2000/svg" | ||
| xmlns:xlink="http://www.w3.org/1999/xlink" | ||
| return html`<svg | ||
| width="100mm" |
There was a problem hiding this comment.
SVG size should match the actual size of the graphics. Otherwise, Wokwi will draw the selection rectangle too big when you click on the element.
There was a problem hiding this comment.
are you suggesting that I should vary the generated svg to match the size of the component?
I can do that
1. applied prettier 2. changed shading to make motor look more metalic 3. changed shaft path to ensure it rotates about its center
AriellaE
left a comment
AriellaE
left a comment
There was a problem hiding this comment.
It looks very good, thank you!
Few more comments:
- Text size of
valuemust always be larger thanunits(the proportions should be around 14:24) , and now that there are size option to the stepper itself, the texts should grow and shrink as the stepper. - In storybook please cancel the rotation animation(although it very pretty) . You can create a new story that contains the animation, but all the other stories (including default story) should be static with the ability of controlling the angle.
src/stepper-motor-element.ts
Outdated
| { name: 'B-', y: 237, x: 127, number: 4, signals: [] }, | ||
| ]; | ||
| render() { | ||
| const spec = this.nemaSpecMap[this.size]; |
There was a problem hiding this comment.
Storybook does validate the size, but in Wokwi users can change the size on the diagram to an invalid value and then they get an error.
1. font size is now different for unit/values 2. removed animation
Modified the
Removed animation altogether. For some reason it does not want to work when I apply to Template type stories (all other stories using animation make direct use of Hope this is it... |
|
@AriellaE - any update? |
|
Hi @bonnyr |
3 participants
Added a new element to represent Steppers based on popular Nema sizes
Example images: