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

refactor: Update form field components to align with the new design spec #842

Merged
merged 11 commits into from
Oct 7, 2024

Conversation

rfgamaral
Copy link
Member

@rfgamaral rfgamaral commented Oct 4, 2024

Short description

This PR implements a few changes to adapt some of the form fields to the new form fields spec. Please refer to CHANGELOG.md (or the commit messages) to better understand the changes that were applied.

Tip

Given the number of changes to bring the form field components to parity with the new form fields spec, a commit-by-commit review is recommended.

Warning

Don't be scared by the number of changes, most of them come from the last commit, which is a temporary commit that includes a Reactist build in this PR branch, so that I can pre-emptively open a PR on ui-extensions and todoist-web without having it break our CI checks.

Before merging this PR, I'll remove that commit, and update both ui-extensions and todoist-web with the real package of Reactist v26.

PR Checklist

  • Updated docs (storybooks, readme)
  • Executed npm run validate and made sure no errors / warnings were shown
  • Described changes in CHANGELOG.md
  • Bumped version in package.json and package-lock.json (npm --no-git-tag-version version <major|minor|patch>) ref
  • Reviewed and approved Chromatic visual regression tests in CI

Versioning

Breaking change, some props were removed from the BaseField component.

@rfgamaral rfgamaral requested a review from a team October 4, 2024 15:35
@rfgamaral rfgamaral self-assigned this Oct 4, 2024
@rfgamaral rfgamaral requested review from pedroalves0 and removed request for a team October 4, 2024 15:35
@rfgamaral rfgamaral changed the title refactor: refactor: Update multiple form field components to align with the new design spec Oct 4, 2024
@rfgamaral rfgamaral changed the title refactor: Update multiple form field components to align with the new design spec refactor: Update multiple form components to align with the new design spec Oct 4, 2024
@rfgamaral rfgamaral changed the title refactor: Update multiple form components to align with the new design spec refactor: Update form field components to align with the new design spec Oct 4, 2024
Copy link
Member

@pedroalves0 pedroalves0 left a comment

Choose a reason for hiding this comment

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

I assume that this PR is not meant to completely align the fields with the new design as there are several things missing or different (e.g. tooltips, invalid error state icon, search clear button).

Not sure if you're continuing this work into Q4, but if you are, I suggest creating stories that replicate 1:1 the Figma designs. Easier to review and to confirm that the client implementation follows the mockups.

src/base-field/base-field.tsx Show resolved Hide resolved
src/base-field/base-field.tsx Show resolved Hide resolved
@rfgamaral
Copy link
Member Author

I assume that this PR is not meant to completely align the fields with the new design as there are several things missing or different (e.g. tooltips, invalid error state icon, search clear button).

The goal was to simplify the components (remove no longer needed things) and to keep them flexible enough to support the new designs (which they do, unless I missed something) from the consumer side.

Not sure if you're continuing this work into Q4(...)

AFAIK, this work will be in the back burner for Q4, and might get picked up some time next year.

@rfgamaral rfgamaral merged commit cc876ef into main Oct 7, 2024
5 checks passed
@rfgamaral rfgamaral deleted the ricardo/refactor-form-fields branch October 7, 2024 14:09
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