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

Filter fields #286

Closed
wants to merge 2 commits into from
Closed

Filter fields #286

wants to merge 2 commits into from

Conversation

devgioele
Copy link
Contributor

Introduces filter fields that follow the same pattern of form fields. Allows for less boilerplate code. Other filter fields might be added in the future.

@devgioele devgioele self-assigned this Sep 19, 2023
@devgioele devgioele requested a review from alexlanz September 19, 2023 11:32
@github-actions
Copy link

Preview link: https://286.react-ui.aboutbits.dev

Copy link
Member

@alexlanz alexlanz left a comment

Choose a reason for hiding this comment

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

While the review I found some points that started some thoughts in my head if it is a good idea to add these components to ReactUI. These components are really opinionated and contain lots of hardcoded stylings.

I see the benefit of having them, but I think it is a better idea to store these components in a shared place inside the project.

It makes also totally sense to already create them in the boilerplate.

What is your opinion on this @lukasvice?

Copy link
Member

Choose a reason for hiding this comment

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

Can we create a story for these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Can we create a story for these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See the comment above

<SearchField
{...filterProps}
variant={FormVariant.Soft}
className={classNames('w-full grow md:w-auto', className)}
Copy link
Member

Choose a reason for hiding this comment

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

I don't know if it is a good idea to hardcode the classes here.

What's your take on this @lukasvice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is what we always use according to our latest design specification

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but why not use the theme? Note that Tailwind is configured to look for class names only in theme files and not in component files!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad. I didn't understand the comment. Of course I will put them in the theme file

return (
<SelectField
{...filterProps}
variant={isScreenMedium ? FormVariant.Transparent : FormVariant.Soft}
Copy link
Member

Choose a reason for hiding this comment

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

Like this we have no change to:

  1. Adjust screen sizes in a project
  2. Adjust the logic if the soft or transparent should be used for different screen size

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is only the default. The variant can be overwritten to your liking

@devgioele
Copy link
Contributor Author

devgioele commented Sep 19, 2023

While the review I found some points that started some thoughts in my head if it is a good idea to add these components to ReactUI. These components are really opinionated and contain lots of hardcoded stylings.

I see the benefit of having them, but I think it is a better idea to store these components in a shared place inside the project.

It makes also totally sense to already create them in the boilerplate.

Form fields are opinionated too. I think filter forms are just an addition and nothing ground-breaking new.
But I am happy to hear your opinion!

@devgioele
Copy link
Contributor Author

I discussed this with @alexlanz and the conclusion is that we do not add filter fields for the time being

@devgioele devgioele closed this Sep 19, 2023
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.

3 participants