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

DT-1407 - add RadioInput component and replace existing native radio inputs #1670

Merged
merged 8 commits into from
Oct 10, 2023

Conversation

rossedfort
Copy link
Contributor

@rossedfort rossedfort commented Oct 5, 2023

Description & motivation 💭

Adds RadioInput and RadioGroup components following the new component spec and replaces existing native <input type="radio" /> with the new Component(s)

Some implementation notes:
<RadioInput /> can be used with or without a <RadioGroup />. To use it standalone, a name and group prop must be passed to each <RadioInput />

<script>
  const group = writable<'a' | 'b' | 'c'>('a');
</script>

<RadioInput id="option-a" value="a" label="Option A" name="options" {group} />
<RadioInput id="option-b" value="b" label="Option B" name="options" {group} />
<RadioInput id="option-c" value="c" label="Option C" name="options" {group} />

to use with a <RadioGroup />, name and group can be passed as props to <RadioGroup /> and will be set on child <RadioInput />'s via context.

<script>
  const group = writable<'a' | 'b' | 'c'>('a');
</script>

<RadioGroup name="options" {group}>
  <RadioInput id="option-a" value="a" label="Option A" />
  <RadioInput id="option-b" value="b" label="Option B" />
  <RadioInput id="option-c" value="c" label="Option C" />
</RadioGroup>

Note: binding to group is not necessary, as the value is a store. group had to be made a store, because context is not reactive by default. For reactivity in svelte context, a reactive value must be used.

Screenshots (if applicable) 📸

Screenshot 2023-10-05 at 10 05 04 AM Screenshot 2023-10-05 at 10 05 30 AM Screenshot 2023-10-05 at 10 05 53 AM

Design Considerations 🎨

Testing 🧪

How was this tested 👻

  • Manual testing
  • E2E tests added
  • Unit tests added

Steps for others to test: 🚶🏽‍♂️🚶🏽‍♀️

Radio inputs were used in 3 places

  • DataEncoderSettings
  • WorkflowResetForm
  • DatetimeFilter

Test for no regressions in functionality in these 3 areas.

Checklists

Draft Checklist

Merge Checklist

Issue(s) closed

Docs

Any docs updates needed?

@vercel
Copy link

vercel bot commented Oct 5, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
holocene ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 10, 2023 3:53pm

@rossedfort rossedfort marked this pull request as ready for review October 5, 2023 16:13
Copy link
Contributor

@laurakwhit laurakwhit left a comment

Choose a reason for hiding this comment

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

🎉

let endpoint = $codecEndpoint ?? '';
let port = $dataConverterPort ?? '';
let passToken = $passAccessToken ?? false;
let includeCreds = $includeCredentials ?? false;
let override = $overrideRemoteCodecConfiguration ?? false;
let override = writable($overrideRemoteCodecConfiguration);
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious why you've chosen writable over ⬇️ here

Suggested change
let override = writable($overrideRemoteCodecConfiguration);
let override = $overrideRemoteCodecConfiguration ?? false;

and if we want to change the other variables to stores as well 🤔

Copy link
Contributor Author

@rossedfort rossedfort Oct 10, 2023

Choose a reason for hiding this comment

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

I did it this way because override needs to be a Writable, and let override = $overrideRemoteCodecConfiguration would make override the value of the store, i.e. a boolean. Additionally, let override = overrideRemoteCodecConfiguration (without the $) would assign override by reference and thus the store value would still be mutated.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah forgot about the context r.e. the group needing to be a store in the note for this PR, thanks!

@rossedfort rossedfort merged commit 15cfcbc into main Oct 10, 2023
9 checks passed
@rossedfort rossedfort deleted the DT-1407-radio-input branch October 10, 2023 16:52
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.

2 participants