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

feat: add option to override react config. #254

Closed
wants to merge 4 commits into from

Conversation

btimby
Copy link

@btimby btimby commented Aug 24, 2021

I have updated README.md, I am testing this change locally right now. I will follow up with results shortly.

Description

  • Adds an option for html-template to define react config.
  • Can be used to disable sidebar.

Related issue(s)

#253

@btimby btimby changed the title Add option to override react config. feat: add option to override react config. Aug 24, 2021
@btimby
Copy link
Author

btimby commented Aug 24, 2021

I added a unit test and manually verified that the sidebar is successfully removed.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@btimby
Copy link
Author

btimby commented Aug 24, 2021

This should be good to go. I checked how github renders the Markdown and I am happy with these changes.

@magicmatatjahu magicmatatjahu self-requested a review September 2, 2021 08:50
Copy link
Member

@magicmatatjahu magicmatatjahu left a comment

Choose a reason for hiding this comment

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

@btimby Hello! Sorry for that late review 😅 I didn't notice that you created that (probably I missed this PR in notifications on Github, sometimes I get 50 a day)... really sorry.

I added some suggestions, great work btw and thanks for contribution :)

@@ -38,6 +38,7 @@ npm install -g @asyncapi/generator
| singleFile | Set output into one html-file with styles and scripts inside | No | `false` | `true`,`false` | `true` |
| outFilename | The filename of the output file. | No | `index.html` | *Any* | `asyncapi.html` |
| pdf | Generates output HTML as PDF | No | `false` | `true,false` | `false` |
| config | Stringified JS object containing options for react | No | null | *Any* | {"show":{"sidebar":false}} |
Copy link
Member

Choose a reason for hiding this comment

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

Please add link to the React component's config shape, something like:

Suggested change
| config | Stringified JS object containing options for react | No | null | *Any* | {"show":{"sidebar":false}} |
| config | Stringified JS object containing [options for the React component](https://github.com/asyncapi/asyncapi-react/blob/next/docs/configuration/config-modification.md#definition) | No | null | *Any* | {"show":{"sidebar":false}} |

@@ -100,6 +100,11 @@
"pdf": {
"description": "Set to `true` to get index.pdf generated next to your index.html",
"default": false
},
"config": {
"description": "Stringified JS object containing options for react, ex: {\"show\":{\"sidebar\":false}}.",
Copy link
Member

Choose a reason for hiding this comment

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

Please fix the description from my suggestion.

Comment on lines +13 to +16
let config = { show: { sidebar: true }, sidebar: { showOperations: 'byDefault' } };
if (params.config) {
config = {...config, ...JSON.parse(params.config) };
}
Copy link
Member

Choose a reason for hiding this comment

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

I think that we should merge the show part of config with the default. In your proposition if someone pass as config:

{ "show": { "operations": false } }

Then operations will be hidden, but sidebar will also hidden, but by default should be showed - only explicit defined sidebar: false should hide the sidebar, so please change it something like (maybe you will come up with something more better):

Suggested change
let config = { show: { sidebar: true }, sidebar: { showOperations: 'byDefault' } };
if (params.config) {
config = {...config, ...JSON.parse(params.config) };
}
let config = { show: { sidebar: true }, sidebar: { showOperations: 'byDefault' } };
if (params.config) {
const parsedCustomConfig = JSON.parse(params.config);
config = {...config, ...parsedCustomConfig, show: { ...config.show, ...(parsedCustomConfig.show || {}) } };
}

Comment on lines +116 to +122
it('should allow override of config', async () => {
const params = { config: '{"show": {"sidebar":false}}' };
const expected = '{"show":{"sidebar":false},"sidebar":{"showOperations":"byDefault"}}';

const result = stringifyConfiguration(params);
expect(result).toEqual(expected);
});
Copy link
Member

Choose a reason for hiding this comment

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

Please also add one test with checking my proposition with deep merging of the config prop :)

@magicmatatjahu
Copy link
Member

@btimby Hi. Would you like to continue working on this PR in near future? :)

@magicmatatjahu
Copy link
Member

@btimby That PR is still valid?

@github-actions
Copy link

github-actions bot commented May 4, 2022

This pull request has been automatically marked as stale because it has not had recent activity 😴

It will be closed in 120 days if no further activity occurs. To unstale this pull request, add a comment with detailed explanation.

There can be many reasons why some specific pull request has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model.

Let us figure out together how to push this pull request forward. Connect with us through one of many communication channels we established here.

Thank you for your patience ❤️

@magicmatatjahu
Copy link
Member

Handled by #363 @btimby Thanks for contribution! In #363 we used your base code.

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.

2 participants