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.

Sign up for GitHub

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

Jump to bottom

Use an option usePathInSidebar #1805

Merged
merged 5 commits into from
Dec 8, 2021
Merged

Use an option usePathInSidebar #1805

merged 5 commits into from
Dec 8, 2021

Conversation

kjhman21
Copy link
Contributor

@kjhman21 kjhman21 commented Nov 26, 2021 •
edited
Loading

What/Why/How?

We found an requirement that we want the path to be shown in the sidebar.
If this option is enabled, it displays a path in the sidebar instead of summary.

Usage:

                <RedocStandalone
                spec={props.spec}
                options={{
                    scrollYOffset:'.navbar',
                    nativeScrollbars: true,
                    sideNavStyle: 'summary-only' or 'path-only'
                }}

Reference

Testing

Screenshots (optional)

sideNavStyle == 'summary-only'
image

sideNavStyle == 'path-only'
image

Check yourself

  • Code is linted
  • Tested
  • All new/updated code is covered with tests

@kjhman21
Use an option usePathInSidebar
e16fa80 
If this option is enabled, it displays a path in the sidebar instead of
summary.

Usage:

```js
                <RedocStandalone
                spec={props.spec}
                options={{
                    scrollYOffset:'.navbar',
                    nativeScrollbars: true,
                    usePathInSidebar:true
                }}
```
@AlexVarchuk AlexVarchuk self-requested a review November 26, 2021 13:46
AlexVarchuk
AlexVarchuk requested changes Nov 26, 2021
View reviewed changes
Copy link
Collaborator

@AlexVarchuk AlexVarchuk left a comment

Choose a reason for hiding this comment

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

Hi @kjhman21 . Thanks for your contribution. Can you rename usePathInSidebar to sideNavStyle. It will be enum of summary-only and path-only with default value summary-only. We will add more options in the future.

@kjhman21
Copy link
Contributor Author

@AlexVarchuk Thanks for the quick review! Updated as you requested. Please take another look. :)

@AlexVarchuk AlexVarchuk self-requested a review November 29, 2021 12:15
RomanHotsiy
RomanHotsiy requested changes Nov 29, 2021
View reviewed changes
Copy link
Member

@RomanHotsiy RomanHotsiy left a comment

Choose a reason for hiding this comment

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

Thanks @kjhman21! It looks great! 💙

I left just a few tiny suggestions. Thanks in advance.

src/services/RedocNormalizedOptions.ts Outdated
@@ -189,6 +206,7 @@ export class RedocNormalizedOptions {
disableSearch: boolean;
onlyRequiredInSamples: boolean;
showExtensions: boolean | string[];
sideNavStyle: string;
Copy link
Member

Choose a reason for hiding this comment

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

But would be awesome to use string enums here: https://www.typescriptlang.org/docs/handbook/enums.html#string-enums

Suggested change
sideNavStyle: string;
sideNavStyle: 'summary-only' | 'path-only'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@RomanHotsiy
Actually, I didn't find string enums in the project. Could you suggest a good location to store enum definition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or, just your suggestion is enough?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, keep it as my suggestion for now. We'll add enums later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@RomanHotsiy It looks ugly, so I applied string enums. Please take another look. Thanks!

src/services/models/Operation.ts Outdated
Comment on lines 108 to 109
this.sidebarName = this.name;
if (options.sideNavStyle === 'path-only') this.sidebarName = this.path;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
this.sidebarName = this.name;
if (options.sideNavStyle === 'path-only') this.sidebarName = this.path;
this.sidebarName = options.sideNavStyle === 'path-only' ? this.path : this.name;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

applied as you suggested.

src/services/MenuStore.ts Outdated
@@ -16,6 +16,7 @@ export interface IMenuItem {
id: string;
absoluteIdx?: number;
name: string;
sidebarName: string;
Copy link
Member

Choose a reason for hiding this comment

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

I would rename it to sidebarLabel

Copy link
Contributor Author

Choose a reason for hiding this comment

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

applied as you suggested.

RomanHotsiy
RomanHotsiy approved these changes Nov 30, 2021
View reviewed changes
Copy link
Member

@RomanHotsiy RomanHotsiy left a comment

Choose a reason for hiding this comment

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

👏 Thanks @kjhman21. Great job!

@AlexVarchuk merge whenever you're ready.

@kjhman21
Copy link
Contributor Author

kjhman21 commented Dec 2, 2021

@RomanHotsiy @Oprysk @AlexVarchuk Thanks for approving this!

@AlexVarchuk When can this be merged? I am really excited to use this feature on my project!

@Oprysk Oprysk merged commit 2e4663b into Redocly:master Dec 8, 2021
@kjhman21 kjhman21 deleted the optionPath branch December 8, 2021 13:54
@Oprysk
Copy link
Contributor

Oprysk commented Dec 9, 2021

@kjhman21 Sorry for the delay, we are going to release it today!

@AlexVarchuk AlexVarchuk mentioned this pull request Mar 14, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Oprysk Oprysk Oprysk approved these changes

@AlexVarchuk AlexVarchuk AlexVarchuk approved these changes

@RomanHotsiy RomanHotsiy RomanHotsiy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kjhman21 @Oprysk @RomanHotsiy @AlexVarchuk