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

fix: hiding highlights for custom courses #1191

Merged
merged 3 commits into from
Apr 3, 2024
Merged

fix: hiding highlights for custom courses #1191

merged 3 commits into from
Apr 3, 2024

Conversation

kiram15
Copy link
Contributor

@kiram15 kiram15 commented Mar 29, 2024

When we have groups feature turned on and they have a group that doesn't apply to all contexts, we do not want to show the highlights feature in the sidebar.

Jira ticket

@kiram15 kiram15 requested a review from a team March 29, 2024 07:20
Copy link

codecov bot commented Mar 29, 2024

Codecov Report

Attention: Patch coverage is 73.33333% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 85.46%. Comparing base (f85826b) to head (1425dbb).

❗ Current head 1425dbb differs from pull request most recent head ef3b251. Consider uploading reports for the commit ef3b251 to get more accurate results

Files Patch % Lines
src/components/Sidebar/index.jsx 83.33% 2 Missing ⚠️
src/data/services/LmsApiService.js 33.33% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1191      +/-   ##
==========================================
- Coverage   85.47%   85.46%   -0.01%     
==========================================
  Files         508      508              
  Lines       11060    11074      +14     
  Branches     2326     2329       +3     
==========================================
+ Hits         9453     9464      +11     
- Misses       1563     1566       +3     
  Partials       44       44              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/containers/Sidebar/Sidebar.test.jsx Outdated Show resolved Hide resolved
@@ -201,6 +224,7 @@ Sidebar.propTypes = {
enableAnalyticsScreen: PropTypes.bool,
onWidthChange: PropTypes.func,
isMobile: PropTypes.bool,
enterpriseGroupsV1: PropTypes.bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Would help readability to have a name that's a bit more explicit that it is a feature flag, IMO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it follows the same naming pattern as the other features, and you can see thats its being brought in from enterpriseFeatures

@kiram15 kiram15 merged commit a19fe04 into master Apr 3, 2024
4 checks passed
@kiram15 kiram15 deleted the kiram15/ENT-8694 branch April 3, 2024 14:26
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