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

individual property readonly support #1763

Merged
merged 2 commits into from
Jun 24, 2021
Merged

Conversation

onerzafer
Copy link
Contributor

Hello I'm using your awsome library and I needed read-only support for individual properties then I found a related issue #1735

I've tried to add the feature. If you can give some feedback and point me in the correct direction I would be happy.

@CLAassistant
Copy link

CLAassistant commented Jun 16, 2021

CLA assistant check
All committers have signed the CLA.

@sdirix
Copy link
Member

sdirix commented Jun 16, 2021

Hi @onerzafer, thank you very much for the contribution! I'll take a look.

Copy link
Member

@sdirix sdirix left a comment

Choose a reason for hiding this comment

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

In general the code is fine :) Thank you again!

I would prefer to adhere to the algorithm presented in #1735, see the respective comment. I'm open for other suggestions, so if you prefer a different algorithm please elaborate why.

Also we definitely need test cases for the isInherentlyEnabled and ideally an example in the examples package which showcases all of them.

packages/core/src/util/runtime.ts Outdated Show resolved Hide resolved
@onerzafer onerzafer requested a review from sdirix June 16, 2021 13:34
@onerzafer
Copy link
Contributor Author

onerzafer commented Jun 16, 2021

Hello, @sdirix I've noticed that there are a couple of tests that are failing on some packages in the CI. I've tried to run them locally but somehow I couldn't manage it. Probably I couldn't install all the packages (test runner is complaining about missing packages etc.) I've read the developer docs also and followed the steps yet we are here me asking about how-tos. How can I be sure my local environment is up and running without any problem and I can test locally? Have you ever encountered such problems before? is it just me? Thanks

@sdirix
Copy link
Member

sdirix commented Jun 17, 2021

Hi @onerzafer, you should be able to run the tests via npm ci && npm run init && npm run build && npm run test. On Linux you should be able to use Node >= 12 while on Mac or Windows it needs to be Node 12 exactly. This is also how our CI is configured.

packages/core/src/util/runtime.ts Outdated Show resolved Hide resolved
@onerzafer onerzafer requested a review from sdirix June 17, 2021 13:12
@onerzafer
Copy link
Contributor Author

onerzafer commented Jun 18, 2021

Hello, @sdirix do you think it is worth merging? Do you have more feedback? I'd love to hear :D

Copy link
Member

@sdirix sdirix left a comment

Choose a reason for hiding this comment

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

We're coming closer to merging but there are still some issues.

packages/examples/src/readonly.ts Outdated Show resolved Hide resolved
packages/core/src/util/runtime.ts Outdated Show resolved Hide resolved
packages/core/src/util/cell.ts Outdated Show resolved Hide resolved
packages/core/test/util/cell.test.ts Outdated Show resolved Hide resolved
packages/core/test/util/runtime.test.ts Show resolved Hide resolved
packages/core/test/util/runtime.test.ts Show resolved Hide resolved
@onerzafer onerzafer requested a review from sdirix June 19, 2021 09:04
@coveralls
Copy link

coveralls commented Jun 21, 2021

Coverage Status

Coverage decreased (-0.007%) to 88.706% when pulling e7d9fa2 on onerzafer:master into 2594df3 on eclipsesource:master.

@onerzafer
Copy link
Contributor Author

Hello, @sdirix I hope I'm not bothering you. I feel like this is ready but I'm not totally sure since I have no deeper understanding of the codebase yet. This is the core package and obviously needs to be more critical while merging. May I ask if you think it's done or needs more work? And from an optimistic point of view in case we managed to merge it, is it realistic to expect a release afterwards anytime soon? Thanks again for bearing with me. Cheers

@sdirix
Copy link
Member

sdirix commented Jun 23, 2021

Hi @onerzafer! I'll take a detailed look in the near future, trying to break the new functionality. It's probably merged within the next days in case we didn't overlook something major (don't think so). We'll also release a 3.0 alpha version in the same time frame which will then probably contain this change.

Öner Zafer and others added 2 commits June 23, 2021 18:06
The JSON Forms core utilities will now also consider the JSON Schema
'readOnly' property as well as 'readonly' and 'readOnly' properties
defined in ui schemas.

The algorithm checks in the following order whether the form wide
readonly flag is set, an enablement rule is defined for the ui schema
element, ui schema or config options are set and whether the JSON Schema
specifies 'readOnly'. If none of them apply the enablement of the parent
is used.

Includes example and test cases.
Update cell `enabled` determination to the new enablement algorithm.

Note that the cell `enabled` still prefers `ownProps` (except for the
form wide readonly flag) over any other determination for performance
gains. The Table (or other renderers dispatching to cells) usually
already determined whether the cell shall be enabled and therefore hand
the correct `enabled` prop over. If that is not wanted, the new
enablement algorithm will kick in when not setting this prop.

Also adds more `isInherentlyEnabled` test cases and fixes the JSON
Schema `readOnly` lookup.
Copy link
Member

@sdirix sdirix left a comment

Choose a reason for hiding this comment

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

LGTM ;)

@sdirix sdirix merged commit 5ccb67e into eclipsesource:master Jun 24, 2021
@sdirix
Copy link
Member

sdirix commented Jun 24, 2021

Hi @onerzafer, JSON Forms 3.0.0-alpha.0 is now released containing the readonly changes. You can consume it by referring to the concrete version or simply use the next tag, for example npm install @jsonforms/core@next @jsonforms/react@next @jsonforms/material-renderers@next.

Thank you again for the contribution :)

@onerzafer
Copy link
Contributor Author

Hi @sdirix this is wunderbar! I appreciate your guidance and effort. Have a good one.

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.

Support readonly defined in JSON Schema and UI Schema
4 participants