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

Setting up syntax highlighting of code section #5259

Conversation

AdrieanKhisbe
Copy link
Contributor

Restore syntax highlighting of the code section

Description

I introduce react-lowlight based on highlight to take care of syntax highliting.

To do so I updated the highlight-code component, and also the curl one.
To modify the curl I had to tweak the copy mecanism. Now instead to click on the area, there is a text Copy Command that will copy command on click.

(an icon where be preferable, but I would need some guidance)

Motivation and Context

Because syntax highligting 🌈 😉
Fixes #3189

How Has This Been Tested?

I tested it locally, and it was working as it should.
I'll see if it breaks the build, and fix it adequately if needed

Screenshots (if appropriate):

Capture d'écran 2019-03-24 16 12 59

Capture d'écran 2019-03-24 17 19 41

Checklist

My PR contains...

  • No code changes (src/ is unmodified: changes to documentation, CI, metadata, etc.)
  • Dependency changes (any modification to dependencies in package.json)
  • Bug fixes (non-breaking change which fixes an issue)
  • Improvements (misc. changes to existing features)
  • Features (non-breaking change which adds functionality)

My changes...

  • are breaking changes to a public API (config options, System API, major UI change, etc).
  • are breaking changes to a private API (Redux, component props, utility functions, etc.).
  • are breaking changes to a developer API (npm script behavior changes, new dev system dependencies, etc).
  • are not breaking changes. (in theory)

Documentation

  • My changes do not require a change to the project documentation.
  • My changes require a change to the project documentation.
  • If yes to above: I have updated the documentation accordingly.

Automated tests

  • My changes can not or do not need to be tested.
  • My changes can and should be tested by unit and/or integration tests.
  • If yes to above: I have added tests to cover my changes.
  • If yes to above: I have taken care to cover edge cases in my tests.
  • All new and existing tests passed.

@AdrieanKhisbe AdrieanKhisbe force-pushed the ft/setting-up-syntax-highlighting branch from f86242b to 0c57874 Compare April 6, 2019 18:36
@AdrieanKhisbe
Copy link
Contributor Author

(rebased)

@AdrieanKhisbe
Copy link
Contributor Author

👋 @shockey
Here is some gentle heads up :)

I know there is right now some conflict, but I'll like some feed back before updating Pull request.

Thanks in advance :)

@AdrieanKhisbe
Copy link
Contributor Author

Hello and Happy new year @shockey & @webron :)

👋 Here is another attempt to get some attention on this Pull Request 😉

I'm awaiting for your feedbacks and possible requested changes to perform the rebase that is needed.

Thanks in advance,
Best regards :)
Adrien

@pfeigl
Copy link

pfeigl commented Mar 24, 2020

Can we please get this merged :-( Having no code highlighting is such a bad experience in an otherwise so beautiful tool.

It's all there, ready to be used, it just takes a review and a merge :-)

@AdrieanKhisbe
Copy link
Contributor Author

@shockey and @shockey, ready to update this PR if you have time to look over it :)

@octobocto
Copy link

I would love for this to get merged, a really needed feature!

@jedsmallwood
Copy link

I am interested in this capability. It just seems like it would make for a better looking UI.

@AdrieanKhisbe AdrieanKhisbe force-pushed the ft/setting-up-syntax-highlighting branch from 0c57874 to e9e9ce4 Compare June 12, 2020 15:28
@AdrieanKhisbe
Copy link
Contributor Author

👋 @shockey & @webron or other like @tim-lai and @swaggerhub-bot ,

I updated the PR to solve conflict that arose during last year.

I really hope you will consider the PR and I will try to reach you on other channel.

It's still working,
Capture d’écran 2020-06-12 à 17 24 49

however the build would likely fail:

npm run build now crash as it enforce webpack performance apprently introduced in #5454.

Capture d’écran 2020-06-12 à 17 34 45

@tim-lai
Copy link
Contributor

tim-lai commented Jun 12, 2020

@AdrieanKhisbe Thanks for the PR! Catching up on long standing PRs... This looks promising. A couple of notes/concerns:

  1. Needs configurability, specifically to enable/disable
  2. The react-lowlight package hasn't been updated in 2+ years, even though highlight.js has received multiple updates this year. We would want a more actively maintained project, especially for non-critical use.
  3. The bundle size is an issue. I recognize our current bundle size doesn't give you much room to work with, but not sure how much or when we would be able to perform another size audit.
  4. I noticed that you removed 'highlight' from /core/utils. That's good. However, copyToClipboard should go into its own file.

@tim-lai tim-lai self-assigned this Jun 13, 2020
@AdrieanKhisbe
Copy link
Contributor Author

Hello @tim-lai ! :)
Thanks for your answers.

To address your concerns, here is what I have in mind:

  1. About configurability sure, I'll see how to introduce opt-out ability, and style if possible (currently hardcoded in html css import). This would be added to SwaggerUIBundle defaults and forwarded to the HighlightCode component

  2. react-lowlight has not been maintained since I made the PR. I'll look for an alternative. I haven't dig much so far, but react-syntax-highlighter, is likely to be the first candiidate.

  3. Would you consider bundling highlighting related dependencies (highlight.js and co) in a dedicated file/entry as an acceptable option?

  4. I'll isolate copyToClipboard in a dedicated file

What do you think about these?

@tim-lai
Copy link
Contributor

tim-lai commented Jun 16, 2020

@AdrieanKhisbe

  1. About configurability sure, I'll see how to introduce opt-out ability, and style if possible (currently hardcoded in html css import). This would be added to SwaggerUIBundle defaults and forwarded to the HighlightCode component

Thinking about this more, it would better to write this as a plugin. Then we could be more agnostic to the libraries used, switch out libraries as needed, and potentially even offer users a choice of highlighters. The plugin could have its own set of config options.

  1. react-lowlight has not been maintained since I made the PR. I'll look for an alternative. I haven't dig much so far, but react-syntax-highlighter, is likely to be the first candiidate.

It's not a hard "no" for react-lowlight right now. Since you already have react-lowlight working, I would start with react-lowlight in a plugin.

  1. Would you consider bundling highlighting related dependencies (highlight.js and co) in a dedicated file/entry as an acceptable option?

We may do code-splitting in the future.

@AdrieanKhisbe
Copy link
Contributor Author

@tim-lai I'm really not sure it should be a plugin 🤔

This should come built-in, and work out-of-the-box (as it was for swagger v1).
Otherwise users would have to add the extra dependency, and then configure the SwaggerUIBundle, and so on. (which might very likely not happen)

For some extra feature opting for a plugin is totally legitimate. However for syntax highlighting of curl command and payloads, I'm really not sure. In my opinion it's really at the core of the experience swagger-ui brings.

Also, I think far more efforts are needed to convert this to a plugin, than to replace react-lowlight by react-syntax-highlighter, and address bundling size issue. (that could be simple with rsh Light Build)

@tim-lai
Copy link
Contributor

tim-lai commented Jun 16, 2020

@AdrieanKhisbe Fair comments. Let's move forward without the plugin concept. Thanks.

@AdrieanKhisbe
Copy link
Contributor Author

Okey @tim-lai, I will go forward based on your inputs :)
I'll ping you on the PR once I have something addressing points 1,2,4 and maybe 3.
Possibly before the week ends.

Also, I might rewrite the whole branch. If you prefer me to keep the original commits, don't hesitate to tell me. :)
(I'll keep a bookmark branch with current state on my fork anyway)

AdrieanKhisbe added a commit to AdrieanKhisbe/swagger-ui that referenced this pull request Jun 17, 2020
@AdrieanKhisbe AdrieanKhisbe force-pushed the ft/setting-up-syntax-highlighting branch from e9e9ce4 to 5329129 Compare June 19, 2020 15:55
@AdrieanKhisbe
Copy link
Contributor Author

Hello @tim-lai,

I did rewrite the PR using react-syntax-highlight, 🌈
I adapted curl copy to use same lib introduced by #5278 📋
I introduced the configurability you suggested (activation & theme) ⚙️
and I bump by a few KiB the max size ⚖️

Documentation is still be done, I wait for a validation on the approach and signature. 📖

Also, choice of themes could be review or extended. 🎨

@tim-lai
Copy link
Contributor

tim-lai commented Jun 23, 2020

@AdrieanKhisbe Functionally, this looks good, and it's also good that it's not pulling from a cdn. Looking forward to the documentation and screenshots.

@AdrieanKhisbe
Copy link
Contributor Author

Okey @tim-lai, I'll handle that then! :)
(And I'll rebase to resolve package-lock conflict)

I might come back to you if I'm not sure about were to add documentation. 📖 (I will start to search from /docs repo folder)

adding react-syntaxt-highligth in a25aefc tiped the scale over limit by very little.
This commit adjust limit. (2.4% increase)
Screenshot was compressed to have similar size than previous swagger-ui3.png
@AdrieanKhisbe AdrieanKhisbe force-pushed the ft/setting-up-syntax-highlighting branch from 5329129 to 0ed23c5 Compare July 3, 2020 14:12
@AdrieanKhisbe
Copy link
Contributor Author

Hello @tim-lai :)

I took care of the conflict via a rebase, and also:

Tell me if you think of other places that should be updated.

@frantuma frantuma added this to the M4 milestone Jul 9, 2020
tim-lai pushed a commit to tim-lai/swagger-ui that referenced this pull request Jul 17, 2020
@AdrieanKhisbe AdrieanKhisbe deleted the ft/setting-up-syntax-highlighting branch July 20, 2020 07:22
@AdrieanKhisbe
Copy link
Contributor Author

Thanks a lot @tim-lai for you help to have to land!! :)
🚀

AdrieanKhisbe added a commit to CoorpAcademy/swagger-ui-express that referenced this pull request Jul 21, 2020
@SledgeHammer01
Copy link

  1. About configurability sure, I'll see how to introduce opt-out ability, and style if possible (currently hardcoded in html css import). This would be added to SwaggerUIBundle defaults and forwarded to the HighlightCode component

@AdrieanKhisbe is there opt-out ability for the syntax highlighting? kind of ugly imo, sorry :).

@AdrieanKhisbe
Copy link
Contributor Author

Hello @SledgeHammer01,
it is indeed possible to opt-out, documentation about was also introduced via this Pull request.

Most simple way is to add syntaxHighlight: false to your config object.
(Note that you can also modify the Highlight theme to see if one is more to your liking)

For more details, see last settings of display section in documentation

@mariem-89
Copy link

mariem-89 commented Sep 24, 2020

Hi @AdrieanKhisbe,

I am using the following configuration which doesn't seem to disable the syntaxHighlight. (swagger-ui version 3.34.0)
Can you please add sample working config to disable the syntaxHighlight?

Thank you

window.onload = function() {
    // Begin Swagger UI call region
    const ui = SwaggerUIBundle({
        url: "https://petstore.swagger.io/v2/swagger.json",
        dom_id: '#swagger-ui',
        deepLinking: true,
        syntaxHighlight: false,
        presets: [
            SwaggerUIBundle.presets.apis,
            SwaggerUIStandalonePreset
        ],
        plugins: [
            SwaggerUIBundle.plugins.DownloadUrl
        ],
        layout: "StandaloneLayout"
    })
    // End Swagger UI call region

    window.ui = ui
}

@AdrieanKhisbe
Copy link
Contributor Author

@mariem-89, the syntaxHighlight: false is supposed to be working. (I checked if there was no changes since the PR landed)

Could you try the explicit: syntaxHighlight: {activated: false}, and tell me what happens?

@mariem-89
Copy link

mariem-89 commented Sep 26, 2020

@AdrieanKhisbe,

In fact after several tests, i can confirm it is not working in the request Body section. But i was only able to disable the syntax Highlight of the response section:

As you can see in the following screenshot.

Do you have any solution or workaround ?

image

I guess there is also the parameter name is not correct in the documentation](https://github.com/swagger-api/swagger-ui/blob/master/docs/usage/configuration.md#parameters). But correct me if am wrong.
image
Which should be: syntaxHighlight.activated

Thank you for your help

@AdrieanKhisbe
Copy link
Contributor Author

Indeed @mariem-89 there seems to be a glitch for the body section, config might not be forwarded correctly or something like.
I'll try do have a look if I can quickly spot where things goes wrong

@AdrieanKhisbe
Copy link
Contributor Author

Unfortunately I don't reproduce neither on my old branch (the one of that PR), nor on current master.

Just adding syntaxHighlight: false in dev-helpers/index.html make it work with npm run dev

What version are you using exactly?
I'm intrigued in your capture about the position of the contact type select:
see the two capture: yours vs the one I just made on master (was the same on AdrieanKhisbe:ft/setting-up-syntax-highlighting)
Capture d’écran 2020-09-27 à 20 03 28
Capture d’écran 2020-09-27 à 20 04 05

@mariem-89
Copy link

@AdrieanKhisbe,

Thank you for your quick reply.
I am using directly the last tag of the swagger-ui: v3.34.0 from the dist folder.
I confirm it's working with the petstore sample (index.html file).
But when i pass a custom OpenAPI definition (defition below):

openapi: 3.0.1
info:
  title: OpenAPI definition
  version: v0
servers:
  - url: 'http://localhost:8080'
    description: Generated server url
paths:
  /values/data:
    post:
      tags:
        - hello-controller
      operationId: list
      requestBody:
        content:
          application/json:
            schema:
              $ref: '#/components/schemas/TrackerData'
      responses:
        '200':
          description: OK
          content:
            '*/*':
              schema:
                $ref: '#/components/schemas/TrackerData'
components:
  schemas:
    TrackerData:
      required:
        - timestamp
        - trackerId
        - value
      type: object
      properties:
        trackerId:
          type: string
          example: the-tracker-id
        timestamp:
          type: string
          format: date-time
          example: '2018-01-01T00:00:00Z'
        value:
          type: number
          description: The data value
          format: double
          example: 19

I get the following result, where the syntax highlighting has not been disabled on the example value of the RequestBody:

image

@AdrieanKhisbe
Copy link
Contributor Author

AdrieanKhisbe commented Oct 1, 2020

Update: you can skip this message to next one 😁

🤔 Strange

Maybe it's linked to the dist that is versioned in the repo.

Though, I retrieved master, and just editing dist/index.html to add syntaxHighlight: false, to SwaggerUIBundle config
and that worked correctly just open dist/index.html
Capture d’écran 2020-10-01 à 18 57 13
Capture d’écran 2020-10-01 à 18 58 04
Capture d’écran 2020-10-01 à 18 54 47

You might note @mariem-89 the position of the select box for request body that is not in the same location

@AdrieanKhisbe
Copy link
Contributor Author

@mariem-89 I just managed to reproduce, adding your spec to dist, serving the files and updating url

Capture d’écran 2020-10-01 à 19 05 57

I will look what is specific to that spec, and where is the non standard path in which config is not injected. (which results in default behavior to activate the syntax highlight).
That's likely related to the multiple content type for request body. (which also explain change in UI)

AdrieanKhisbe added a commit to AdrieanKhisbe/swagger-ui that referenced this pull request Oct 1, 2020
… Examples 🔌

As a result, syntax highlight could not be disabled be disabled or configured in that elements
AdrieanKhisbe added a commit to AdrieanKhisbe/swagger-ui that referenced this pull request Oct 1, 2020
…o Examples 🔌

As a result, syntax highlight could not be disabled be disabled or configured in that elements
@AdrieanKhisbe
Copy link
Contributor Author

@mariem-89 I founded the cause, Example(s) were not being forwarded the configuration.
I've just Corrected that, the Pull request is here: #6455 🙂

@mariem-89
Copy link

@AdrieanKhisbe,

Thank you very much for your help. Really appreciated.
Let's wait for the PR to be approved and merged!

tim-lai pushed a commit that referenced this pull request Oct 2, 2020
* Complement #5259, getConfigs was not correctly forwarded to Examples. As a result, syntax highlight could not be disabled be disabled or configured in that elements
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.

Feature request: Syntax Highlighting
9 participants