Adds showNavbar and showSearchButton props to CourseTabsNavigationSlot#1932
Adds showNavbar and showSearchButton props to CourseTabsNavigationSlot#1932jacobo-dominguez-wgu wants to merge 2 commits into
Conversation
…onSlot - Add showNavbar prop to conditionally render the nav bar - Add showSearchButton prop to conditionally render the search toggle
|
Thanks for the pull request, @jacobo-dominguez-wgu! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
openedx-webhooks
added
open-source-contribution
jacobo-dominguez-wgu
requested review from
brian-smith-tcril,
diana-villalvazo-wgu and
xitij2000
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1932 +/- ##
==========================================
+ Coverage 91.30% 91.32% +0.01%
==========================================
Files 346 346
Lines 5776 5784 +8
Branches 1350 1358 +8
==========================================
+ Hits 5274 5282 +8
Misses 483 483
Partials 19 19 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Could you update the README for the slot to include the props (and possibly an example using them)? |
Sure. I have updated the slot readme with props docs and examples. |
brian-smith-tcril
left a comment
brian-smith-tcril
left a comment
There was a problem hiding this comment.
I'm not sure this is the direction we should go with this slot.
The comments I left are coming from a "we want to land these pluginProps" perspective, but I'd like to think big picture a bit first.
The version of CourseTabsNavigationSlot in master right now
export const CourseTabsNavigationSlot = ({ tabs, activeTabSlug }: CourseTabsNavigationProps) => (
<PluginSlot id="org.openedx.frontend.learning.course_tabs_navigation.v1">
<CourseTabsNavigation tabs={tabs} activeTabSlug={activeTabSlug} />
</PluginSlot>
);has no pluginProps, meaning someone who wanted to replace the CourseTabsNavigation component wouldn't have access to activeTabSlug or tabs.
If the goal of this is to just hide everything but the search dialog (as the "Hide the navigation bar" example shows), then I think that can be accomplished with the slot as-is (without adding any props):
import { DIRECT_PLUGIN, PLUGIN_OPERATIONS } from '@openedx/frontend-plugin-framework';
import { CoursewareSearch } from '@src/course-home/courseware-search';
import { useCoursewareSearchState } from '@src/course-home/courseware-search/hooks';
const CourseTabsNavigation = () => {
const { show } = useCoursewareSearchState();
return (
<div id="courseTabsNavigation" className="mb-3 course-tabs-navigation">
{show && <CoursewareSearch />}
</div>
);
};
const config = {
pluginSlots: {
"org.openedx.frontend.learning.course_tabs_navigation.v1": {
keepDefault: false,
plugins: [
{
op: PLUGIN_OPERATIONS.Insert,
widget: {
id: 'custom_tab',
type: DIRECT_PLUGIN,
RenderWidget: CourseTabsNavigation
},
},
],
},
},
}
export default config;| it('should NOT render CoursewareSearch if the flag is off', () => { | ||
| renderComponent(); | ||
|
|
||
| expect(screen.queryByTestId('courseware-search-dialog')).not.toBeInTheDocument(); | ||
| expect(screen.queryByRole('dialog')).not.toBeInTheDocument(); | ||
| }); |
There was a problem hiding this comment.
I'm on board with testing by role instead of test id, but when I first saw this change it made me think it was needed for the tests to pass. Is the test id still being used by other tests? If not, it could be removed (and then the "why is this changing?" question would have a clear answer: "the test id was removed")
| {showNavbar && ( | ||
| <div className="container-xl"> | ||
| <div className="nav-bar"> | ||
|
|
There was a problem hiding this comment.
| </div> | ||
| </div> | ||
| )} | ||
| {show && <CoursewareSearch />} |
There was a problem hiding this comment.
It'd be good to rename show now that there are 2 other shows being used in here.
Alright, this example perfectly works for our use case. Thank you! |
5 participants
Description
Adds showNavbar and showSearchButton props to the CourseTabsNavigationSlot, allowing operators to hide the course tab links and/or search toggle button through plugins framework without replacing the entire slot.
Configuration example