search

Authress-Engineering / openapi-explorer

OpenAPI Web component to generate a UI from the spec.
Apache License 2.0
316 stars 42 forks source link

Adding highlighting to XML examples #186

Closed j3rem1e closed 1 year ago

j3rem1e commented 1 year ago

This PR add highlighting to xml examples in response:

highlight_xml

Moreover, some css properties used by prism were not defined from the theme to the css variable. I have fixed that too.

j3rem1e commented 1 year ago

So, just some small things to change. If anything I think the biggest thing missing here is the answer to the question, why does the api-request execute response use this to display:

does that work for XML, or does that need to be changed too? If it does work, then we should be able to do the same thing here and not explicitly pass markup to the Prism, but rather the responseType. And if it doesn't work then we'll need some sort of prismResolver.js to handle our fixes to prism to handle types that it doesn't support out of the box.

Afaik it already "worked" before this PR. There is a mapping between content types and prism languages hardcoded here : https://github.com/Rhosys/openapi-explorer/blob/962e21f59eda5b535d34ba6eba2ed5bef5f01e9f/src/components/api-request.js#L564

It "worked" in the DOM, but it wasn't visible to the end user because of the css variables and styles missing. (that's why I added "language-markup" explicitly during my test, but it was already included by default in prism.js - I should remove it),

I agree it's kind of duplicated ^^ Imo, there should be some kind of "content-viewer" component which is responsable to render examples/responses + content-type, to fixes some inconsistencies. For example:

I have hesitated to make such change, but it was just to big for this kind of PR ^^

wparad commented 1 year ago

So, just some small things to change. If anything I think the biggest thing missing here is the answer to the question, why does the api-request execute response use this to display: does that work for XML, or does that need to be changed too? If it does work, then we should be able to do the same thing here and not explicitly pass markup to the Prism, but rather the responseType. And if it doesn't work then we'll need some sort of prismResolver.js to handle our fixes to prism to handle types that it doesn't support out of the box.

Afaik it already "worked" before this PR. There is a mapping between content types and prism languages hardcoded here :

https://github.com/Rhosys/openapi-explorer/blob/962e21f59eda5b535d34ba6eba2ed5bef5f01e9f/src/components/api-request.js#L564

It "worked" in the DOM, but it wasn't visible to the end user because of the css variables and styles missing. (that's why I added "language-markup" explicitly during my test, but it was already included by default in prism.js - I should remove it),

I agree it's kind of duplicated ^^ Imo, there should be some kind of "content-viewer" component which is responsable to render examples/responses + content-type, to fixes some inconsistencies. For example:

  • Why is the "json-viewer" not used on response ? Moreover, the style of prism and json-viewer don't match
  • Why is there no "copy button" on examples ?
  • And there are conflict between the prismcss and other style, like the ".header" class - It should probably be scoped into its own component

I have hesitated to make such change, but it was just to big for this kind of PR ^^

I wouldn't worry about answering any of those questions yet, it's more of a question of why and not "we should fix it", for starters, we use the responseType in the API request - response object, I hope we can also use the responseType here to cause it to render the right thing. I think a good starting point would be to refactor the api request response to a new template and have it generate the result for the example, that feels like an easy/small change. If it isn't it would be good to know why.

It "worked" in the DOM, but it wasn't visible to the end user because of the css variables and styles missing. (that's why I added "language-markup" explicitly during my test, but it was already included by default in prism.js - I should remove it),

In that case, if this is already mostly can be aligned then a refactored component would be even easier, and I'd be all for pulling that in. This is still small, and avoiding a digression here would help align future changes as well. It's okay if you aren't up for it, but at the very least I would hope these two rendering use almost the exact same code.

Why is the "json-viewer" not used on response ? Moreover, the style of prism and json-viewer don't match

Ehh, things drift, but this might actually be the right question, and if possible maybe that's the way to align this, idk, you are right something is definitely off here. The one thing I would like to avoid is further drift. What do you want to do?

j3rem1e commented 1 year ago

I have reworked this PR : Now all the logic behind highlighting is in only one place, included the "copy to clipboard" button

wparad commented 1 year ago

I only took a quick look at this, but from what I've seen it looks reasonable. As soon as I get a moment, I'll do a full review.

j3rem1e commented 1 year ago

Mostly just a couple of small things. So we are just about ready to get this in. The only thing left on my mind is if the syntaxt-highlighter really should be merged with the json-tree. I can't figure out a reason we would use one without the other, it's right now a "wrapper for a wrapper", but I'll let you decide if you want to merge these, I don't have a strong opinion either way.

I don't know the best practices of lit-element, but to me component should be small and testable. I don't think it's better to merge them.

wparad commented 1 year ago

I don't know the best practices of lit-element, but to me component should be small and testable. I don't think it's better to merge them.

It's fine to have separate files that doesn't change anything, but when we have lit elements wrap each other, that creates new shadow DOM that doesn't pass through the css, so we have to reinject all our css classes which is annoying. Also things like parameters to the syntax-highlighter if it had a lot (like the api-request) then passing them all in, just to then pass them to the json-tree would also not be the best. For now, it's fine, and we'll just have to keep an eye on it, however I don't expect it to unnecessarily grow, so it shouldn't be a problem.