Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

Migrate all checkbox inputs to the CheckBoxInput component #16813

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

Conversation

@Devansu-Yadav
Copy link
Contributor

@Devansu-Yadav Devansu-Yadav commented Mar 12, 2023 •
edited by gtsiolis
Loading

Description

  • Migrating to the CheckBoxInput component instead of CheckBox component for consistency.

Related Issue(s)

Fixes #16768

How to test

Release Notes

Migrate all checkbox inputs to the new CheckBoxInput component for consistency

Documentation

Build Options:

  • /werft with-werft
    Run the build with werft instead of GHA
  • leeway-no-cache
  • /werft no-test
    Run Leeway with --dont-test
Publish Options
  • /werft publish-to-npm
  • /werft publish-to-jb-marketplace
Installer Options
  • with-ee-license
  • with-dedicated-emulation
  • with-ws-manager-mk2
  • workspace-feature-flags
    Add desired feature flags to the end of the line above, space separated

Preview Environment Options:

  • /werft with-local-preview
    If enabled this will build install/preview
  • /werft with-preview
  • /werft with-large-vm
  • /werft with-gce-vm
    If enabled this will create the environment on GCE infra
  • /werft with-integration-tests=all
    Valid options are all, workspace, webapp, ide, jetbrains, vscode, ssh

Copy link

annotations in the pull request changed, but user is not allowed to start a job

@Devansu-Yadav Devansu-Yadav marked this pull request as draft March 12, 2023 16:57
@Devansu-Yadav Devansu-Yadav changed the title (削除) [dashboard] migrate to CheckBoxInput (#16768) (削除ここまで) (追記) Migrate all checkbox inputs to the CheckBoxInput component (追記ここまで) Mar 12, 2023
Copy link
Contributor

gtsiolis commented Mar 13, 2023 •
edited by werft-gitpod-dev-com bot
Loading

/werft run with-preview

👍 started the job as gitpod-build-migrate-checkbox-component-fork.0
(with .werft/ from main)

Copy link
Contributor Author

Devansu-Yadav commented Mar 20, 2023 •
edited by werft-gitpod-dev-com bot
Loading

@gtsiolis This PR is ready for review 🏓, although I wasn't able to test out the UX for the recent change I made in this PR as there are some issues while running the dashboard app locally (have already raised this issue on discord and can confirm that it's not because of the changes made in this PR).

Once the UX is good to go, I'll make sure to delete the old CheckBox component.

Note: Some of the changes in this PR may impact the existing UI of some pages already using the new CheckboxInput component, like the Onboarding flow page, which was updated in #16545.

@Devansu-Yadav Devansu-Yadav force-pushed the migrate-checkbox-component branch 2 times, most recently from 64e7560 to 8f7037a Compare March 25, 2023 13:56
Copy link
Contributor Author

@gtsiolis Any updates on whether the UX for the changes made in this PR is good to go or not?
gtsiolis reacted with eyes emoji

@gtsiolis gtsiolis force-pushed the migrate-checkbox-component branch from 8f7037a to 3be4f04 Compare March 30, 2023 17:56
Copy link

started the job as gitpod-build-migrate-checkbox-component.5 because the annotations in the pull request description changed
(with .werft/ from main)

Copy link
Contributor

gtsiolis commented Mar 30, 2023 •
edited by werft-gitpod-dev-com bot
Loading

/werft run with-preview

👍 started the job as gitpod-build-migrate-checkbox-component-fork.1
(with .werft/ from main)

Copy link
Contributor

gtsiolis commented Mar 30, 2023 •
edited by werft-gitpod-dev-com bot
Loading

Looking at this now! 👀
Devansu-Yadav reacted with heart emoji

Copy link
Contributor

@gtsiolis gtsiolis left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for your patience and pushing this forward, @Devansu-Yadav! 🌮 🌮

UX LGTM! 🏁

💡 tip: I've deployed up a preview environment in https://migrate-ch7cf88b5c76.preview.gitpod-dev.com/workspaces.

1️⃣ Left some minor comments below.

2️⃣ There's one more checkbox that was added recently, posting the lines of code here in case you'd like to address this change, too.

<CheckBox
title={<span>Workspace Sharing</span>}
desc={
<span>
Allow workspaces creаted within an Organization to share the workspace with any authenticated user.
</span>
}
checked={!settings?.workspaceSharingDisabled}
onChange={({ target }) =>
handleUpdateTeamSettings({ workspaceSharingDisabled: !target.checked })
}
disabled={isLoading}
/>

3️⃣ Once all Checkbox components have been replaced, we can remove that file and component. It should be fine to do this separately if you prefer. Your call. 🏓

@selfcontained could you take a diagonal look at the code changes here? 🏀

Devansu-Yadav reacted with thumbs up emoji Devansu-Yadav reacted with heart emoji
Comment on lines 31 to 32
label: string|React.ReactNode;
hint?: string|React.ReactNode;
Copy link
Contributor

Choose a reason for hiding this comment

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

thought: Not sure about this change, but let me loop in @selfcontained who initially added this component.

Devansu-Yadav reacted with thumbs up emoji
Copy link
Contributor Author

@Devansu-Yadav Devansu-Yadav Mar 31, 2023

Choose a reason for hiding this comment

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

This change is made to accommodate for the usage of <CheckBox /> component in certain places (like ProjectSettings) where React nodes are passed as props to specific attributes instead of text.

Copy link
Contributor

@selfcontained selfcontained Apr 10, 2023

Choose a reason for hiding this comment

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

Makes sense, thanks! We can simplify it to just the React.ReactNode type as that allows for the string type as well.

Devansu-Yadav reacted with thumbs up emoji
label="Edit user permissions by adding or removing roles for this user."
className="mt-0"
>
{flags.map((e) => (
Copy link
Contributor

Choose a reason for hiding this comment

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

thought: Highlighting this part for other reviewers. 🏓

Devansu-Yadav reacted with thumbs up emoji
Copy link
Contributor

@selfcontained selfcontained Apr 10, 2023

Choose a reason for hiding this comment

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

This should be rop.map(... so it shows the roles, not flags.

Devansu-Yadav reacted with thumbs up emoji
Copy link
Contributor Author

Devansu-Yadav commented Mar 31, 2023 •
edited
Loading

💡 tip: I've deployed up a preview environment in https://migrate-ch7cf88b5c76.preview.gitpod-dev.com/workspaces.

Thanks for this!

1️⃣ Left some minor comments below.

Sure, I have left a comment to explain those changes :)

2️⃣ There's one more checkbox that was added recently, posting the lines of code here in case you'd like to address this change, too.

Sure, I have pushed a commit for this.

<CheckBox
title={<span>Workspace Sharing</span>}
desc={
<span>
Allow workspaces creаted within an Organization to share the workspace with any authenticated user.
</span>
}
checked={!settings?.workspaceSharingDisabled}
onChange={({ target }) =>
handleUpdateTeamSettings({ workspaceSharingDisabled: !target.checked })
}
disabled={isLoading}
/>

3️⃣ Once all Checkbox components have been replaced, we can remove that file and component. It should be fine to do this separately if you prefer. Your call. 🏓

@gtsiolis Sure, I would also prefer to remove the old CheckBox component files in a separate PR.

Copy link
Contributor

gtsiolis commented Mar 31, 2023 •
edited
Loading

I would also prefer to remove the old CheckBox component files in a separate PR.

DEAL—Sounds great, @Devansu-Yadav! No need to do this here. Opened #17103 to keep track of this. 🍊
Devansu-Yadav reacted with thumbs up emoji

Copy link
Contributor

@gtsiolis gtsiolis left a comment •
edited
Loading

Choose a reason for hiding this comment

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

UX LGTM. 🌮 🌮

Approving to unblock merging, holding to let someone from the @gitpod-io/engineering-webapp team to take a closer look at the code changes. 🏓

Devansu-Yadav reacted with thumbs up emoji Devansu-Yadav reacted with heart emoji
Copy link
Contributor

@svenefftinge svenefftinge left a comment

Choose a reason for hiding this comment

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

Code LGTM

Devansu-Yadav reacted with heart emoji
@gtsiolis gtsiolis removed the request for review from selfcontained April 6, 2023 12:08
Copy link
Contributor

gtsiolis commented Apr 6, 2023 •
edited by werft-gitpod-dev-com bot
Loading

Thanks for taking a look, @svenefftinge. 🌮 🌮

Removing hold and starting a build from the workspace.

/unhold
Devansu-Yadav reacted with heart emoji

Copy link
Contributor

svenefftinge commented Apr 6, 2023 •
edited by werft-gitpod-dev-com bot
Loading

/werft run

👍 started the job as gitpod-build-migrate-checkbox-component-fork.2
(with .werft/ from main)

Copy link
Member

aledbf commented Apr 7, 2023

/hold

(blocking the merge queue)

Copy link
Contributor

gtsiolis commented Apr 7, 2023 •
edited by werft-gitpod-dev-com bot
Loading

/werft run with-preview

👍 started the job as gitpod-build-migrate-checkbox-component-fork.3
(with .werft/ from main)

Copy link
Contributor

@selfcontained selfcontained left a comment

Choose a reason for hiding this comment

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

Thank you so much for helping with these changes 😄 I left a few pieces of feedback that would great to address as part of this before we merge it.

Devansu-Yadav reacted with thumbs up emoji gtsiolis and Devansu-Yadav reacted with heart emoji
<span
className={classNames(
"text-sm",
"text-md",
Copy link
Contributor

@selfcontained selfcontained Apr 10, 2023

Choose a reason for hiding this comment

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

I see why this change was made, but I don't think we want to bump the input field hint font size across the board. Having it as text-sm is intentional, especially noticeable our other input fields where we want it to be a slightly smaller font size.

For now can we keep this as text-sm and if we decided we'd like larger text specifically for checkboxes we can either add a property to InputFieldHint for that variance, or not use InputFieldHint on the checkbox component. Let's leave that as a followup and keep it as text-sm please though.

Devansu-Yadav reacted with thumbs up emoji

<CheckboxInputContainer>
<CheckboxInput
value="Block Users"
Copy link
Contributor

@selfcontained selfcontained Apr 10, 2023

Choose a reason for hiding this comment

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

Could we make value optional on CheckboxInput, and then we can leave it off in cases like this where it isn't beneficial to set.

Devansu-Yadav reacted with thumbs up emoji
Copy link
Contributor Author

@Devansu-Yadav Devansu-Yadav Apr 11, 2023

Choose a reason for hiding this comment

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

@selfcontained Sure, I'll make the change. Is there anywhere else where I should drop the value prop?

Copy link
Contributor

@selfcontained selfcontained Apr 11, 2023

Choose a reason for hiding this comment

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

I think you should be safe to drop the value prop from all the new places in this PR where we've swapped to the new component if they weren't using it before (I think it's all of them).

label="Edit user permissions by adding or removing roles for this user."
className="mt-0"
>
{flags.map((e) => (
Copy link
Contributor

@selfcontained selfcontained Apr 10, 2023

Choose a reason for hiding this comment

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

This should be rop.map(... so it shows the roles, not flags.

Devansu-Yadav reacted with thumbs up emoji
Copy link
Contributor Author

Devansu-Yadav commented Apr 11, 2023 •
edited by werft-gitpod-dev-com bot
Loading

Thank you so much for helping with these changes 😄 I left a few pieces of feedback that would great to address as part of this before we merge it.

@selfcontained I have addressed almost all the changes you suggested. Do let me know if I missed something 😄
gtsiolis reacted with rocket emoji

@selfcontained selfcontained changed the base branch from main to bmh/devansu-migrate-checkbox-component April 11, 2023 22:22
Copy link
Contributor

selfcontained commented Apr 11, 2023 •
edited
Loading

Thanks @Devansu-Yadav for all your help here 😄

I'm going to update this PR to merge into a non-main branch (bmh/devansu-migrate-checkbox-component) in this repo so that we can work around some things with our build pipeline that we're working through still. I'll go ahead and make the remaining adjustments (removing value from a few more spots), and then we'll get this into main.

Update: New PR w/ some tweaks here.
Devansu-Yadav reacted with thumbs up emoji Devansu-Yadav reacted with heart emoji

@selfcontained selfcontained merged commit 4ac6332 into gitpod-io:bmh/devansu-migrate-checkbox-component Apr 11, 2023
roboquat pushed a commit that referenced this pull request Apr 12, 2023 
* Migrate all checkbox inputs to the `CheckBoxInput` component (#16813)
* [dashboard] migrate to CheckBoxInput (#16768)
* [dashboard] remove CheckBox comp usage
* [dashboard] remove `CheckBox` on team settings
* [dashboard] remove border on checkbox click state
* [dashboard] refactor code for `CheckboxInput`
* removing values on checkboxes that don't need it
* adjusting api a bit
* simplify a bit by always setting value
---------
Co-authored-by: Devansu Yadav <devansuyadav@gmail.com>
Copy link
Contributor Author

Devansu-Yadav commented Apr 12, 2023 •
edited by werft-gitpod-dev-com bot
Loading

Update: New PR w/ some tweaks here.

@selfcontained Just went through this PR and the code looks a whole lot better now! Thanks for the tweaks 🙌

@Devansu-Yadav Devansu-Yadav deleted the migrate-checkbox-component branch April 12, 2023 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@selfcontained selfcontained Awaiting requested review from selfcontained

2 more reviewers

@gtsiolis gtsiolis gtsiolis approved these changes

@svenefftinge svenefftinge svenefftinge approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Migrate Checkbox components to the new CheckboxInputField component

AltStyle によって変換されたページ (->オリジナル) /