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

coral: implement form component [#141] #159

Merged
merged 12 commits into from
Nov 1, 2022
Merged

Conversation

programmiri
Copy link
Contributor

@programmiri programmiri commented Oct 27, 2022

Resolves: #141

About this change - What it does

Adds a Form.tsx component based on a solution created for aiven-core.

Why this way

  • The aiven-core solution is becoming its own OS package once it's stable enough. Until then we will mirror the code there, while aiven-core is always the single source of truth.
  • Our Form.tsx only implements the functionality we need for Klaw.
  • I had to add a little tweak for lodash usage:
    • changes in ts-config to enable import from "lodash/get" with setting esModuleInterop to true. Otherwise jest won't be able to handle it.
    • removed allowSyntheticDefaultImports: true because that's set to true when using esModuleInterop
    • matched imported version from lodash and types to the one used in aiven-design system

ℹ️ for review

  • I tried to wrap related changes in separate commits
  • The second last commit from today adds more explicit typing and removes use of any
  • In this commit I also changed eslint setting so use of any is now an error

@programmiri programmiri linked an issue Oct 27, 2022 that may be closed by this pull request
@programmiri programmiri force-pushed the 141-implement-form-component branch 2 times, most recently from b08ea2a to 10b4736 Compare October 28, 2022 07:59
@programmiri programmiri marked this pull request as ready for review October 28, 2022 08:00
@programmiri programmiri requested review from a team as code owners October 28, 2022 08:00
@programmiri programmiri self-assigned this Oct 28, 2022
@programmiri programmiri force-pushed the 141-implement-form-component branch from 10b4736 to deceda4 Compare October 28, 2022 08:54
Copy link
Contributor

@SmuliS SmuliS left a comment

Choose a reason for hiding this comment

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

LGTM, but would be great if we would render the component in this PR or then we could frop the login components from this PR to the followup.

Also, a rebase it needed.

coral/src/app/components/Form.tsx Outdated Show resolved Hide resolved
coral/package.json Outdated Show resolved Hide resolved
coral/src/app/pages/login/index.tsx Outdated Show resolved Hide resolved
@SmuliS SmuliS changed the title 141 implement form component coral: implement form component [#141] Oct 31, 2022
@testing-library/user-event has a peerDep for @testing-library/dom
but it's a different version than the one that @testing-library/react
uses and brings in as a devDep. Solution based on research on GitHub
shows we can either ignore the warning in pnpm or install the
@testing-library/dom as devDep in the version that @testing-library/react
uses it. I did the latter hoping that in one of the next versions of
@testing-library/user-event this will be resolved.

see
testing-library/user-event#438 (comment)
testing-library/user-event#551 (comment)
- remove use of 'any' type
- add stricter rule for use of any for linting
- `index.tsx` should be reserved for resource root
- files for pages can live on first level in `pages`
- rename files to match  naming conventions for react
- update documentation to be more explicit and correct
@programmiri programmiri force-pushed the 141-implement-form-component branch from b3f54d6 to 2d27275 Compare October 31, 2022 17:07
@programmiri programmiri requested a review from SmuliS October 31, 2022 17:08
│ │ ├── page-one
│ │ ├── ...
│ │ └── index.ts
│ │ ├── index.tsx
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think want to even try to keep this up to date with current implementation so lets keep the examples abstract.

Copy link
Contributor

Choose a reason for hiding this comment

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

But we can do this later 👍

@SmuliS SmuliS merged commit 57eb19c into main Nov 1, 2022
@SmuliS SmuliS deleted the 141-implement-form-component branch November 1, 2022 12:29
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.

🪸 Implement Form component based on Aiven console
2 participants