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

feat: rework Select to use reactstrap implementation #970

Merged
merged 17 commits into from
Dec 4, 2023

Conversation

federico-ntr
Copy link
Contributor

@federico-ntr federico-ntr commented Jun 16, 2023

Fixes #914

PR Checklist

  • My branch is up-to-date with the Upstream master branch.
  • The unit tests pass locally with my changes (if applicable).
  • I have added tests that prove my fix is effective or that my feature works (if applicable).
  • I have added necessary documentation (if appropriate).

Short description of what this resolves:

Changes proposed in this Pull Request:

@vercel
Copy link

vercel bot commented Jun 16, 2023

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

Name Status Preview Comments Updated (UTC)
design-react-kit ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 4, 2023 9:16am

Copy link
Member

@dej611 dej611 left a comment

Choose a reason for hiding this comment

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

Code review pass

import React, { ChangeEvent, ReactElement } from 'react';
import { Input } from 'reactstrap';

export interface SelectProps {
Copy link
Member

Choose a reason for hiding this comment

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

This should extends the native HTML select element props, similar to the Input component:

export interface InputProps extends InputHTMLAttributes<HTMLInputElement> {

Something like:

Suggested change
export interface SelectProps {
export interface SelectProps extends HTMLSelectAttributes<HTMLSelectElement> {

/**
* L'identificativo univoco del componente
*/
id: string;
Copy link
Member

Choose a reason for hiding this comment

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

Not required if extending the native element

* @param selectedValue
* @returns
*/
handleChange: (selectedValue: string) => void;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
handleChange: (selectedValue: string) => void;
onChange: (selectedValue: string) => void;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem with this is that SelectHTMLAttributes already has an onChange property with a different signature, which accepts an event instead of a string

Copy link
Contributor Author

@federico-ntr federico-ntr Jun 26, 2023

Choose a reason for hiding this comment

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

Actually, it has also a handleChange, I found a way to override it but it's not really elegant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I implemented the aforementioned way to override the signature (using Omit<K,V>) but feel free to to suggest better alternatives

</div>
);
export const Select = ({
id = 'select',
Copy link
Member

Choose a reason for hiding this comment

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

I think it is not a good idea to have a default id value, as id is something that should be unique in a DOM tree.
If the only reason to have it is to use it as reference for the label tag, I could suggest branching the rendering based on the id value:

  • if none is passed, then the label tag will wrap the Input component
  • otherwise use the current structure (without the -select suffix)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with you, my bad for not knowing label could wrap its component

src/Select/Select.tsx Outdated Show resolved Hide resolved
src/Select/Select.tsx Outdated Show resolved Hide resolved
src/Select/Select.tsx Outdated Show resolved Hide resolved
stories/Form/Select.stories.mdx Outdated Show resolved Hide resolved
stories/Form/Select.stories.tsx Outdated Show resolved Hide resolved
stories/Form/Select.stories.mdx Outdated Show resolved Hide resolved
@dej611
Copy link
Member

dej611 commented Jun 21, 2023

Also, need to add a note on the Changelog page about the component change and breaking changes.
Possibly a guide on how to migrate from the Select 4.x to the Select 5.x would be nice, but not mandatory.

@federico-ntr
Copy link
Contributor Author

Also, need to add a note on the Changelog page about the component change and breaking changes. Possibly a guide on how to migrate from the Select 4.x to the Select 5.x would be nice, but not mandatory.

I think this belongs to another PR, since nobody has started writing it yet, not even for already implemented features.

Copy link
Collaborator

@sabato-galasso sabato-galasso left a comment

Choose a reason for hiding this comment

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

It is okay for me

@astagi astagi merged commit 5124f2c into italia:next Dec 4, 2023
2 of 4 checks passed
@sabato-galasso sabato-galasso mentioned this pull request Dec 4, 2023
3 tasks
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.

4 participants