Skip to content

Open api async api ux support #368

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

Merged
merged 22 commits into from
Nov 18, 2022
Merged

Open api async api ux support #368

merged 22 commits into from
Nov 18, 2022

Conversation

hasanheroglu
Copy link
Contributor

@hasanheroglu hasanheroglu commented Nov 8, 2022

Closes #365

@netlify
Copy link

netlify bot commented Nov 8, 2022

Deploy Preview for playground-testing ready!

Name Link
🔨 Latest commit 12225f4
🔍 Latest deploy log https://app.netlify.com/sites/playground-testing/deploys/63776d51f32f880009ae7ddb
😎 Deploy Preview https://deploy-preview-368--playground-testing.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.

@egekorkan
Copy link
Member

egekorkan commented Nov 11, 2022

Some todos:

  • The print button should be changed to something like the downwards arrow button of browsers.
  • The tests should not use examples. The test cases should be in another directory
  • Export as URL button can be grouped with the download button, with a chain icon. This button should become unavailable when the async API or open API is selected
  • JSON YAML button to be changed to a checkbox where the JSON and YAML text are both visible. YAML becomes disabled for TD and automatically reverts back to JSON. Going back to open or async API stays in JSON.
  • Package-lock conflict should be resolved
  • Testing the YAML conversion of TDs would be nice https://json-ld.github.io/yaml-ld/spec is in its infancy but why not. The conversion should be bidirectional. Also see Convert JSON-LD to YAML-LD using standard YAML libraries json-ld/yaml-ld#12

@egekorkan
Copy link
Member

Title attributes for all buttons would be nice. Also the JSON/YAML button is not the same size as the ones next to it

@egekorkan
Copy link
Member

We can try the YAML support with https://www.npmjs.com/package/js-yaml . It can be that some TDs cannot be converted back to JSON but let's try and see which cases break.

We should put a warning to the left of the button saying the feature is experimental.

@egekorkan
Copy link
Member

egekorkan commented Nov 16, 2022

The tests are actually passing but the netlify preview shows a console error that results in the page failing to load. No clear how a require is not defined but I am guessing a scoping issue?

Copy link
Member

@egekorkan egekorkan left a comment

Choose a reason for hiding this comment

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

Some problems I have detected.

  • Yaml conversion should be in the core since the CLI can use it in the future too
  • If the initial TD is invalid JSON, converting to YAML has a weird behavior. I think this is due to line 164 in util.js
    • invalid JSON should not even show the warning this is experimental pop up

@egekorkan
Copy link
Member

Also, saving as a link has a weird behavior in yaml. A link is generated but it cannot be used

@egekorkan
Copy link
Member

@hasanheroglu it is looking pretty good. I did one minor change and will merge it once the tests pass.

@egekorkan
Copy link
Member

I have noticed two issues:

  1. converting a TD to YAML and then switching to TM -> the editor shows a yaml TM but the editor thinks it should be json and shows a json parsing error
  2. this makes the open API and async API features invisible unless someone puts a relevant td. Open API is fine since people will probably put an HTTP TD but I think the tabs should always be there but not clickable unless the protocol is detected. While they are inactive, a title attribute should say "Please insert a TD which uses HTTP" and "Please insert a TD which uses MQTT" for Open API and Async API respectively.

@egekorkan
Copy link
Member

@hasanheroglu nice work! Merging this nice feature (set of features now ^_^)

@egekorkan egekorkan merged commit 407baf3 into eclipse-thingweb:master Nov 18, 2022
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.

Protocol Detection
2 participants