Remove ribbon when technology list is empty, or matches root path #4
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
remove ribbon for when technology is empty, matches technology
dobromir-hristov
requested changes
Mar 4, 2022
| // show modules if page | ||
| // 1. belongs to multiple technologies | ||
| // 2. if name doesn't match root of page | ||
| // const root = this.identifier.split('/documentation/')[-1].split('/')[0]; |
Collaborator
There was a problem hiding this comment.
Suggested change
| // const root = this.identifier.split('/documentation/')[-1].split('/')[0]; |
| showSummary({ shouldShowLanguageSwitcher, showmodules }) { | ||
| // get setting | ||
| const hideSummary = getSetting(['features', 'docs', 'summary', 'hide'], false); | ||
| return !hideSummary && (shouldShowLanguageSwitcher || showmodules); |
Collaborator
There was a problem hiding this comment.
Should we add extendsTechnology too?
| :isSymbolDeprecated="isSymbolDeprecated" | ||
| :isSymbolBeta="isSymbolBeta" | ||
| :languagePaths="languagePaths" | ||
| :rootPath="technology.title" |
Collaborator
There was a problem hiding this comment.
Probably rootTitle is a better name, since we dont pass a path?
| expect(wrapper.find(Summary).exists()).toBe(false); | ||
| }); | ||
|
|
||
| it('renders the Summary, if more than 1 module', () => { |
Collaborator
There was a problem hiding this comment.
I dont see a test to hide the summary.
marinaaisa
requested changes
Mar 4, 2022
Collaborator
marinaaisa
left a comment
marinaaisa
left a comment
There was a problem hiding this comment.
I added some refactor suggestions :)
| ), | ||
| shouldShowLanguageSwitcher: ({ objcPath, swiftPath }) => objcPath && swiftPath, | ||
| hideSummary: () => getSetting(['features', 'docs', 'summary', 'hide'], false), | ||
| showModules() { |
Collaborator
There was a problem hiding this comment.
I suggest simplifying this computed property as:
showModules({ modules, rootPath }) {
// show modules if page belongs to multiple technologies or if name doesn't match root of page
if (!modules) return false;
return rootPath !== modules[0].name || modules.length > 1;
},
Collaborator
There was a problem hiding this comment.
Need to make sure modules is not an empty array
| return (this.modules && this.modules.length > 1) | ||
| ? true : (this.modules && this.rootPath !== this.modules[0].name); | ||
| }, | ||
| showSummary({ shouldShowLanguageSwitcher, showmodules }) { |
Collaborator
There was a problem hiding this comment.
Suggested change
| showSummary({ shouldShowLanguageSwitcher, showmodules }) { | |
| showSummary({ shouldShowLanguageSwitcher, showModules }) { |
| showSummary({ shouldShowLanguageSwitcher, showmodules }) { | ||
| // get setting | ||
| const hideSummary = getSetting(['features', 'docs', 'summary', 'hide'], false); | ||
| return !hideSummary && (shouldShowLanguageSwitcher || showmodules); |
Collaborator
There was a problem hiding this comment.
Suggested change
| return !hideSummary && (shouldShowLanguageSwitcher || showmodules); | |
| return !hideSummary && (shouldShowLanguageSwitcher || showModules); |
| it('renders the Summary, if more than 1 module', () => { | ||
| const modules = ['FooKit', 'BarKit']; | ||
| wrapper.setProps({ modules }); | ||
| // wrapper = shallowMount(DocumentationTopic, { propsData }); |
Collaborator
There was a problem hiding this comment.
Suggested change
| // wrapper = shallowMount(DocumentationTopic, { propsData }); |
Owner
Author
|
Issue was fixed with: #8 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Bug/issue #, if applicable: 88214559, 89672818
Summary
Testing
Steps:
To test #1, go to a
SlothCreatorpage.To test #2, if you have access to a documentation archive that includes a page where the “framework” metadata does not match the root framework in the breadcrumb, navigate to the page and verify that the ribbon is not rendered.
Checklist
npm test, and it succeeded