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

[change] Allow multiple jsonschema widgets in one page #333

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nemesifier
Copy link
Member

Earlier the schema.json was bound to each view. No matter how many
different switcher were present, it was only loading one of them.

With this patch, fetching schema.json is bound to each switcher element.
This allows fetching different JSONSchemas on the same page.

Earlier the schema.json was bound to each view. No matter how many
different switcher were present, it was only loading one of them.

With this patch, fetching schema.json is bound to each switcher element.
This allows fetching different JSONSchemas on the same page.
Copy link
Member Author

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

@pandafy I found a minor issue while testing the widget in the credentials edit page, can you replicate it?

Screenshot from 2020-12-04 10-42-35

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0008%) to 99.174% when pulling a53c16c on improve-jsonwidget into fcfa337 on master.

Copy link
Member

@pandafy pandafy left a comment

Choose a reason for hiding this comment

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

@pandafy I found a minor issue while testing the widget in the credentials edit page, can you replicate it?

Yes, I am able to replicate it and it is occurring on master branch as well. I have opened #334 for it.

I believe it is unrelated to this PR.

@nemesifier
Copy link
Member Author

@pandafy I found a minor issue while testing the widget in the credentials edit page, can you replicate it?

Yes, I am able to replicate it and it is occurring on master branch as well. I have opened #334 for it.

I believe it is unrelated to this PR.

@pandafy you're right, so I think we can merge this one. Do you agree?

Copy link
Member Author

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

@pandafy shouldn't this line also be removed as part of this change?

<script>django._jsonSchemaWidgetUrl = "{% url schema_view_name %}";</script>

Copy link
Member

@pandafy pandafy left a comment

Choose a reason for hiding this comment

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

@pandafy shouldn't this line also be removed as part of this change?

<script>django._jsonSchemaWidgetUrl = "{% url schema_view_name %}";</script>

Yes @nemesisdesign , you are right! 😄

We should than also remove this, otherwise test suite will fail.

def test_credentials_jsonschema_widget_presence(self):
url = reverse(f'admin:{self.app_label}_credentials_add')
schema_url = reverse(CredentialsSchemaWidget.schema_view_name)
expected = f'<script>django._jsonSchemaWidgetUrl = "{schema_url}";</script>'
self._login()
response = self.client.get(url)
self.assertContains(response, expected)

@nemesifier
Copy link
Member Author

@pandafy Thanks for your integration.

I have also noticed the advanced mode code in the JS refers to an ID, but the CSS paths should be converted to use classes.

pandafy added a commit to pandafy/openwisp-controller that referenced this pull request Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants