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

Controls: Clean up arg unboxing and switch statements #14394

Merged
merged 5 commits into from Jul 19, 2021

Conversation

@stevensacks
Copy link
Contributor

@stevensacks stevensacks commented Mar 30, 2021

Issue:

What I did

Cleaned up the ArgControl and Options by unboxing the value and using a dict instead of a large switch case.

How to test

It's a minor implementation change and it works in all the examples/official-storybook actions stories.

  • Is this testable with Jest or Chromatic screenshots?
    No

  • Does this need a new example in the kitchen sink apps?
    No

  • Does this need an update to the documentation?
    No

@shilman shilman changed the title fix/arg control cleanup Controls: Clean up arg unboxing Mar 30, 2021
@shilman shilman changed the title Controls: Clean up arg unboxing Controls: Clean up arg unboxing and switch statements Mar 30, 2021
@shilman
Copy link
Member

@shilman shilman commented Apr 16, 2021

@ghengeveld can you take a look at the chromatic changes and let me know what you think?

@ndelangen
Copy link
Member

@ndelangen ndelangen commented Apr 22, 2021

Those changes in chromatic look all right to me, this one seems a bit odd:

Screenshot 2021-04-22 at 17 25 41

Looks like the default value for this arg is a function that returns a string.
I'd think this can't have a control. (how would you edit a function?)

@nx-cloud
Copy link

@nx-cloud nx-cloud bot commented Apr 29, 2021

Nx Cloud Report

CI ran the following commands for commit 4400681. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

Status Command
#000000 nx run-many --target=prepare --all --parallel --max-parallel=15

Sent with 💌 from NxCloud.

const NoControl = () => <>-</>;

export const ArgControl: FC<ArgControlProps> = ({ row, arg, updateArgs }) => {
const { key, control } = row;

const [isFocused, setFocused] = useState(false);
// box because arg can be a fn (e.g. actions) and useState calls fn's
const [boxedValue, setBoxedValue] = useState({ value: arg });
const [value, setValue] = useState(() => arg);
Copy link
Member

@ndelangen ndelangen May 3, 2021

what's the point of doing this:

useState(() => arg);

Copy link
Member

@shilman shilman May 3, 2021

I "boxed" the arg because if the arg was a function it was getting executed, but the boxed version fixes that. However, the function is better than boxing because it doesn't allocate extra memory

Copy link
Member

@ghengeveld ghengeveld May 21, 2021

Please re-add a comment explaining why we're doing "lazy" state initialization here.

Copy link
Member

@shilman shilman Jul 19, 2021

Suggested change
const [value, setValue] = useState(() => arg);
// done this way because the value of `arg` can be a function
const [value, setValue] = useState(() => arg);

@@ -28,28 +28,36 @@ const normalizeOptions = (options: Options, labels?: Record<any, string>) => {
return options;
};

const Controls: Record<string, FC> = {
check: CheckboxControl,
'inline-check': CheckboxControl,
Copy link
Member

@ndelangen ndelangen May 3, 2021

@shilman I think in the future we might want to rename these to check-inline, minor DX improvement maybe. Not 100% sure if it's worth it, but could be an addition so no breaking change, WDYT?

Copy link
Member

@ndelangen ndelangen left a comment

Code changes look good to me.

@ghengeveld ghengeveld assigned stevensacks and shilman and unassigned ghengeveld May 21, 2021
@shilman shilman merged commit 94dcdbb into storybookjs:next Jul 19, 2021
34 of 38 checks passed
@shilman shilman added this to the 6.4 PRs milestone Jul 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants