Skip to content
This repository has been archived by the owner on Jan 9, 2023. It is now read-only.

Checkbox should use isEditable instead of disabled #288

Closed
jackcmeyer opened this issue Feb 20, 2020 · 7 comments
Closed

Checkbox should use isEditable instead of disabled #288

jackcmeyer opened this issue Feb 20, 2020 · 7 comments
Assignees
Labels
bug Something isn't working good first issue Good for newcomers in progress
Milestone

Comments

@jackcmeyer
Copy link
Member

jackcmeyer commented Feb 20, 2020

🐛 Bug Report

The way that Checkbox is enabled/disable is not consistent with other components.

Expected behavior

Instead of a disabled prop, there should be a isEditable prop.

Technical Notes

  • This will be a breaking change.

Related Issues

#62
HospitalRun/hospitalrun#218

@jackcmeyer jackcmeyer added bug Something isn't working help wanted Extra attention is needed good first issue Good for newcomers labels Feb 20, 2020
@jackcmeyer jackcmeyer added this to the v1.0.0 milestone Feb 20, 2020
@Alex-IS
Copy link

Alex-IS commented Feb 20, 2020

I would like to take this task. Also, I guess that expected behavior should be:

Instead of a disabled prop, there should be a isEditable prop.

based on this comment HospitalRun/hospitalrun#218 (comment)

Looking forward to start contributing to this project

@jackcmeyer
Copy link
Member Author

@Alex-IS welcome! And yes you’re right, I flipped it. I’ve updated the issue.

@jackcmeyer jackcmeyer added in progress and removed help wanted Extra attention is needed labels Feb 20, 2020
@matteovivona matteovivona changed the title Chceckbox should use isEditable instead of disabled Checkbox should use isEditable instead of disabled Feb 20, 2020
@jackcmeyer
Copy link
Member Author

jackcmeyer commented Feb 20, 2020

@Alex-IS. Forgive me, I really messed this one up.

The correct implementation should be a disabled prop, so that it's consistent with the rest of the components, which it's already doing. So this issue is no longer needed.

@Alex-IS
Copy link

Alex-IS commented Feb 20, 2020

No problem :-) Should we implement wrapper component in frontend package?

@jackcmeyer
Copy link
Member Author

No problem :-) Should we implement wrapper component in frontend package?

I think this could be a good idea.

@Alex-IS
Copy link

Alex-IS commented Feb 20, 2020

Also, what is the reasoning about having wrapper components in frontend package? I'm trying to understand architecture decisions, so I can be of help

@jackcmeyer
Copy link
Member Author

jackcmeyer commented Feb 20, 2020

The intention of the components repository is for it to be a spot where the most "primitive" components are defined. The advantage is that once the interfaces are defined, then these components can be updated independently without breaking any changes.

At the time, it felt like the "wrapper" components were not "primitive" enough to be defined in the components repository. However, I think they could (and maybe should) be defined in this repository.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working good first issue Good for newcomers in progress
Projects
None yet
Development

No branches or pull requests

2 participants