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

OBPIH-4710 Create reusable filter component #3468

Merged
merged 7 commits into from
Sep 15, 2022
Merged

Conversation

kchelstowski
Copy link
Collaborator

No description provided.


import Translate from 'utils/Translate';

const ButtonPrimary = ({ label, defaultLabel, disabled }) => (
Copy link
Collaborator

Choose a reason for hiding this comment

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

The components ButtonPrimary and ButtonTransparent look identical to me and can probably be a single Button component with props and custom classes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This probably can be a global button component and not strictly connected to Filter

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I agree but wanted to split them, but at one point thought that if they may be easier to use if they are separated.
As for second comment - they are global, I don't know why you think they aren't?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry I thought these button components are inside the Filter directory but they are in form-element which is alright.
But both of the ButtonPrimary and ButtonTransparent should be merged into one Button component that has props to modify it in a way that can create both ButtonPrimary and ButtonTransparent from Button


</div>
<div className="submit-buttons">
<button
Copy link
Collaborator

Choose a reason for hiding this comment

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

Regarding the customButton component that you would be able to use here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, I forgot to delete this section as in the beginning I was just copying form from some other component. It should be (and will be) deleted of course.

const [amountFilled, setAmountFilled] = useState(0);
const countFilled = (values) => {
// Calculate which object's values are not empty
setAmountFilled(Object.keys(values).filter(key => values[key]).length);
Copy link
Collaborator

Choose a reason for hiding this comment

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

A better solution would probably be to do Object.values
Object.values(values).filter((val) => val).length

Copy link
Collaborator

Choose a reason for hiding this comment

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

but also remember about falsy values like 0.
If value of a filter will be 0 it will result as false during the filter and will not be counted

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

that's nice point, I probably didn't consider this as I was only using string-like fields for testing. Thanks for that

@kchelstowski
Copy link
Collaborator Author

@drodzewicz I made <Button> instead of two separate <ButtonPrimary> <ButtonTransparent> and also fixed the issues you mentioned.
I also removed <Button> from FilterForm.md and added it to separate .md file inside form-elements.

@awalkowiak awalkowiak merged commit 3cb72a4 into feature/ui-redesign Sep 15, 2022
@awalkowiak awalkowiak deleted the OBPIH-4710 branch September 15, 2022 12:17
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.

None yet

3 participants