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

add readOnly functionality to array and list #2040

Merged
merged 2 commits into from
Jun 20, 2023

Conversation

LukasBoll
Copy link
Contributor

closes #1901

@coveralls
Copy link

coveralls commented Nov 8, 2022

Coverage Status

Coverage: 84.254%. Remained the same when pulling 7b595cf on LukasBoll:listReadOnly into 1e1ccad on eclipsesource:master.

@sdirix
Copy link
Member

sdirix commented Nov 8, 2022

@LukasBoll With this PR readOnly arrays don't even show their action buttons. Alternatively we could still show them but disable them. What do prefer personally?

@LukasBoll
Copy link
Contributor Author

There are two cases:

  1. The array is set to read only permanently: In this case, it would be better to don’t show any buttons at all, because otherwise the user may expect some functionalities that aren´t supported.

  2. The array can be disabled and enabled: While it would be better to enable/disable the buttons to draw attention to the functionality and the current state, hiding the buttons is still an acceptable solution.

Overall, I would not show the action buttons, since 1) is probably the basic case and it is misleading for the user if buttons without functionality are shown.

@sdirix
Copy link
Member

sdirix commented Nov 15, 2022

@lucas-koehler With this PR readOnly arrays don't even show their action buttons. Alternatively we could still show them but disable them. What do prefer personally?

@lucas-koehler
Copy link
Contributor

@sdirix Ideally,we would support both cases described by @LukasBoll :

  • don't show them for permanent readonly
  • disable them when disabled by a rule

The reason is that I would expect a disabled button to have the possibility to get enabled.

If supporting both is not feasible, I tend to agree with @LukasBoll to not show them to not confuse users in the first case.

@sdirix
Copy link
Member

sdirix commented Dec 5, 2022

@LukasBoll please rebase or merge in the master branch. The files were moved.

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.

It works!

However the validation icons are now moved. Can you adapt the changes so that the icon stays in place?

image

image

@netlify
Copy link

netlify bot commented Feb 21, 2023

Deploy Preview for jsonforms-examples ready!

Name Link
🔨 Latest commit 7b595cf
🔍 Latest deploy log https://app.netlify.com/sites/jsonforms-examples/deploys/644297d94ccc5a0008c098e0
😎 Deploy Preview https://deploy-preview-2040--jsonforms-examples.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

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.

Works for me!

@sdirix sdirix merged commit fa055c2 into eclipsesource:master Jun 20, 2023
@sdirix sdirix mentioned this pull request Nov 14, 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.

Arrays can be modified even with readonly property set on true
4 participants