Skip to content

feat: Add Sidebar and SidebarNotificationButton components for Value Prop - #414

Merged
julianajlk merged 20 commits into
masterfrom
julianajlk/REV-2125/valueprop-sidebar-2
Apr 29, 2021
Merged

feat: Add Sidebar and SidebarNotificationButton components for Value Prop#414
julianajlk merged 20 commits into
masterfrom
julianajlk/REV-2125/valueprop-sidebar-2

Conversation

@julianajlk

@julianajlk julianajlk commented Apr 15, 2021

Copy link
Copy Markdown
Contributor

This is a duplicate of https://github.com/edx/frontend-app-learning/pull/390, which has unknown build failures. This PR is the same but commit 68cf1db is divided up into separate commits here. More commits were added after commit 3bf88c7 Styling/a11y updates.

REV-2125.

For Value Prop implementation, we will implement a sidebar that is triggered on click of a button with upgrade messaging and CTA. Pls note this ticket does not cover adding the upgrade messaging and CTA, it only covers adding the button trigger and sidebar component.

Large screen size (> 992px):
Screen Shot 2021-04-16 at 1 31 05 PM

Screen Shot 2021-04-16 at 1 30 57 PM

Responsive at medium screen sizes (> 576px and < 992px): it has the button like large and above screen size, but the sidebar behaves in full width mode like for small screens.
Screen Shot 2021-04-08 at 12 57 28 PM
Screen Shot 2021-04-08 at 12 57 37 PM

Responsive at small screen sizes (< 576px):
Screen Shot 2021-04-08 at 1 03 12 PM
Screen Shot 2021-04-08 at 1 02 26 PM

@codecov

codecov Bot commented Apr 15, 2021

Copy link
Copy Markdown

Codecov Report

Merging #414 (bc2e75f) into master (3e82152) will increase coverage by 1.24%.
The diff coverage is 91.48%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #414      +/-   ##
==========================================
+ Coverage   82.01%   83.26%   +1.24%     
==========================================
  Files         190      201      +11     
  Lines        3031     3442     +411     
  Branches      753      887     +134     
==========================================
+ Hits         2486     2866     +380     
- Misses        529      556      +27     
- Partials       16       20       +4     
Impacted Files Coverage Δ
src/courseware/course/CourseBreadcrumbs.jsx 95.45% <ø> (ø)
src/courseware/course/NotificationIcon.jsx 66.66% <66.66%> (ø)
src/courseware/course/sequence/Sequence.jsx 89.65% <85.71%> (-0.35%) ⬇️
src/courseware/course/Course.jsx 96.96% <90.00%> (-3.04%) ⬇️
src/courseware/course/Sidebar.jsx 100.00% <100.00%> (ø)
...rc/courseware/course/SidebarNotificationButton.jsx 100.00% <100.00%> (ø)
src/courseware/course/messages.js 100.00% <100.00%> (ø)
...equence/sequence-navigation/SequenceNavigation.jsx 100.00% <100.00%> (ø)
src/generic/tabs/useWindowSize.js 86.66% <100.00%> (+0.95%) ⬆️
...ss-tab/grades/grade-summary/GradeSummaryHeader.jsx 87.50% <0.00%> (-12.50%) ⬇️
... and 24 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3e82152...bc2e75f. Read the comment docs.

@julianajlk
julianajlk force-pushed the julianajlk/REV-2125/valueprop-sidebar-2 branch from 4923832 to 3bf88c7 Compare April 16, 2021 16:09
Comment thread src/courseware/course/Course.jsx Outdated
@julianajlk
julianajlk requested a review from inventhouse April 16, 2021 17:43
Comment thread src/courseware/course/CourseBreadcrumbs.jsx Outdated
Comment thread src/courseware/course/SidebarNotificationButton.jsx Outdated
@julianajlk julianajlk changed the title Add Sidebar and SidebarNotificationButton components for Value Prop feat: Add Sidebar and SidebarNotificationButton components for Value Prop Apr 26, 2021
@julianajlk
julianajlk force-pushed the julianajlk/REV-2125/valueprop-sidebar-2 branch from 68be812 to a44b20c Compare April 27, 2021 15:47

@dianekaplan dianekaplan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No issues jump out at me- though I'm less familiar with this stuff so I'm not sure if they would.

One question would be around the unit test expectations: I see a couple lines flagged that aren't covered by unit tests yet. I that something that'll be included in this PR, or is it done more on a case-by-case basis for frontend-app-learning?

@julianajlk

Copy link
Copy Markdown
Contributor Author

@dianekaplan good question, something I wonder too - overall test coverage went up with this PR and it does meet the minimum for merging, but not sure where to draw the line with UI tests to be honest. I can add some more if that's going to make the code more reliable

@jhan217 jhan217 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks fine to me, couple of minor comments

Comment thread src/courseware/course/Course.jsx Outdated
Comment thread src/courseware/course/Course.jsx Outdated
Comment thread src/courseware/course/Course.jsx Outdated
Comment thread src/courseware/course/Sidebar.jsx Outdated
Comment thread src/courseware/course/Sidebar.jsx
@julianajlk
julianajlk force-pushed the julianajlk/REV-2125/valueprop-sidebar-2 branch from d4ab967 to bc2e75f Compare April 29, 2021 14:17
@julianajlk
julianajlk merged commit 28fddc5 into master Apr 29, 2021
@julianajlk
julianajlk deleted the julianajlk/REV-2125/valueprop-sidebar-2 branch April 29, 2021 15:36
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.

4 participants