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

Add Auto-saving and Search to Lesson Resource Selection #4253

Merged
merged 14 commits into from Sep 20, 2018

Conversation

jonboiser
Copy link
Contributor

@jonboiser jonboiser commented Sep 17, 2018

Summary

  1. Adds a search bar to Lesson Resource Selection
  2. Removes 'Save' button and automatically saves resource changes when selecting resources
  3. Adds a select-all button to topics
  4. Updates copy on UI and different snackbar notifications
  5. Updates some of the copy on the Create Exam Page
  6. Fixes whitespace in .feature files

Search
search

Autosave
autosave

Create Exam Page

screen shot 2018-09-17 at 3 02 58 pm

Reviewer guidance

Testing search

  1. Search term with results
  2. Search term with no results
  3. Search from within UI
  4. Search using URL (put search term at the very of the URL)
  5. Exiting search page should go back to previous page or channels listing

Testing auto save

  1. Test toggling select-all
  2. Toggling single object
  3. Resources are kept when going from topic to topic or preview or search
  4. Resources are kept when exiting the selection workflow

References


Contributor Checklist

  • Contributor has fully tested the PR manually
  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • [ x] If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If there are any front-end changes, before/after screenshots are included
  • If this is an important user-facing change, PR or related issue has a 'changelog' label

Reviewer Checklist

  • Automated test coverage is satisfactory
  • Reviewer has fully tested the PR manually
  • PR has been tested for accessibility regressions
  • External dependencies files were updated (yarn and pip)
  • Documentation is updated
  • Link to diff of internal dependency change is included
  • CHANGELOG.rst is updated for high-level changes
  • Contributor is in AUTHORS.rst

@jonboiser jonboiser added the TODO: needs review Waiting for review label Sep 17, 2018
@jonboiser jonboiser added this to the 0.11.0 milestone Sep 17, 2018
@rtibbles
Copy link
Member

To quote @navakk9: "BEST NEWS EVER!" - this is great!

I know it may not be possible in the scope of the time available to get this in, but would be good to think about how we can try to reduce code duplication for search once we have also implemented this for exams (and also with the core search functionality too).

The other question is about the filtering and pagination - the filtering is now dependent on the displayed results, which can hide relevant filter options if there are more results than the default page size. There's also no option to paginate. The search results include metadata that can be used here, the core search functionality implements these, for reference.

:contentHasCheckbox="contentIsDirectoryKind"
:contentCardMessage="selectionMetadata"
:contentCardLink="contentLink"
@change_select_all="toggleTopicInWorkingResources"

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@codecov
Copy link

codecov bot commented Sep 18, 2018

Codecov Report

Merging #4253 into develop will decrease coverage by 0.07%.
The diff coverage is 20.68%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4253      +/-   ##
===========================================
- Coverage     53.2%   53.13%   -0.08%     
===========================================
  Files          693      697       +4     
  Lines        22046    22158     +112     
  Branches      2985     3007      +22     
===========================================
+ Hits         11729    11773      +44     
- Misses        9622     9685      +63     
- Partials       695      700       +5
Impacted Files Coverage Δ
...assets/src/views/assignments/AssignmentSummary.vue 20% <ø> (ø) ⬆️
...ch/assets/src/views/exams/CreateExamPage/index.vue 0% <ø> (ø) ⬆️
...coach/assets/src/views/lessons/LessonsRootPage.vue 0% <ø> (ø) ⬆️
...nResourceSelectionPage/LessonContentCard/index.vue 0% <ø> (ø) ⬆️
...ach/assets/src/modules/lessonResources/handlers.js 0% <0%> (ø) ⬆️
...iews/lessons/LessonResourceSelectionPage/index.vue 0% <0%> (ø) ⬆️
...ns/LessonResourceSelectionPage/ContentCardList.vue 0% <0%> (ø)
...urceSelectionPage/LessonContentCard/CornerIcon.vue 0% <0%> (ø)
...i/plugins/coach/assets/src/routes/lessonsRoutes.js 0% <0%> (ø) ⬆️
...eSelectionPage/LessonContentCard/CardThumbnail.vue 0% <0%> (ø) ⬆️
... and 11 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 bbc6266...2192061. Read the comment docs.

@codecov
Copy link

codecov bot commented Sep 18, 2018

Codecov Report

Merging #4253 into develop will increase coverage by 0.01%.
The diff coverage is 20.58%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4253      +/-   ##
===========================================
+ Coverage     53.2%   53.21%   +0.01%     
===========================================
  Files          693      699       +6     
  Lines        22046    22255     +209     
  Branches      2985     3024      +39     
===========================================
+ Hits         11729    11843     +114     
- Misses        9622     9710      +88     
- Partials       695      702       +7
Impacted Files Coverage Δ
...assets/src/views/assignments/AssignmentSummary.vue 20% <ø> (ø) ⬆️
...ch/assets/src/views/exams/CreateExamPage/index.vue 0% <ø> (ø) ⬆️
...s/src/views/assignments/AssignmentDetailsModal.vue 82.92% <ø> (ø) ⬆️
...coach/assets/src/views/lessons/LessonsRootPage.vue 0% <ø> (ø) ⬆️
...nResourceSelectionPage/LessonContentCard/index.vue 0% <ø> (ø) ⬆️
...ch/assets/src/views/exams/CoachExamsPage/index.vue 0% <ø> (ø) ⬆️
...ach/assets/src/modules/lessonResources/handlers.js 0% <0%> (ø) ⬆️
...nPage/SearchTools/ResourceSelectionBreadcrumbs.vue 0% <0%> (ø)
...ns/LessonResourceSelectionPage/ContentCardList.vue 0% <0%> (ø)
...urceSelectionPage/LessonContentCard/CornerIcon.vue 0% <0%> (ø)
... and 36 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 bbc6266...2192061. Read the comment docs.

5 similar comments
@codecov
Copy link

codecov bot commented Sep 18, 2018

Codecov Report

Merging #4253 into develop will increase coverage by 0.01%.
The diff coverage is 20.58%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4253      +/-   ##
===========================================
+ Coverage     53.2%   53.21%   +0.01%     
===========================================
  Files          693      699       +6     
  Lines        22046    22255     +209     
  Branches      2985     3024      +39     
===========================================
+ Hits         11729    11843     +114     
- Misses        9622     9710      +88     
- Partials       695      702       +7
Impacted Files Coverage Δ
...assets/src/views/assignments/AssignmentSummary.vue 20% <ø> (ø) ⬆️
...ch/assets/src/views/exams/CreateExamPage/index.vue 0% <ø> (ø) ⬆️
...s/src/views/assignments/AssignmentDetailsModal.vue 82.92% <ø> (ø) ⬆️
...coach/assets/src/views/lessons/LessonsRootPage.vue 0% <ø> (ø) ⬆️
...nResourceSelectionPage/LessonContentCard/index.vue 0% <ø> (ø) ⬆️
...ch/assets/src/views/exams/CoachExamsPage/index.vue 0% <ø> (ø) ⬆️
...ach/assets/src/modules/lessonResources/handlers.js 0% <0%> (ø) ⬆️
...nPage/SearchTools/ResourceSelectionBreadcrumbs.vue 0% <0%> (ø)
...ns/LessonResourceSelectionPage/ContentCardList.vue 0% <0%> (ø)
...urceSelectionPage/LessonContentCard/CornerIcon.vue 0% <0%> (ø)
... and 36 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 bbc6266...2192061. Read the comment docs.

@codecov
Copy link

codecov bot commented Sep 18, 2018

Codecov Report

Merging #4253 into develop will increase coverage by 0.01%.
The diff coverage is 20.58%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4253      +/-   ##
===========================================
+ Coverage     53.2%   53.21%   +0.01%     
===========================================
  Files          693      699       +6     
  Lines        22046    22255     +209     
  Branches      2985     3024      +39     
===========================================
+ Hits         11729    11843     +114     
- Misses        9622     9710      +88     
- Partials       695      702       +7
Impacted Files Coverage Δ
...assets/src/views/assignments/AssignmentSummary.vue 20% <ø> (ø) ⬆️
...ch/assets/src/views/exams/CreateExamPage/index.vue 0% <ø> (ø) ⬆️
...s/src/views/assignments/AssignmentDetailsModal.vue 82.92% <ø> (ø) ⬆️
...coach/assets/src/views/lessons/LessonsRootPage.vue 0% <ø> (ø) ⬆️
...nResourceSelectionPage/LessonContentCard/index.vue 0% <ø> (ø) ⬆️
...ch/assets/src/views/exams/CoachExamsPage/index.vue 0% <ø> (ø) ⬆️
...ach/assets/src/modules/lessonResources/handlers.js 0% <0%> (ø) ⬆️
...nPage/SearchTools/ResourceSelectionBreadcrumbs.vue 0% <0%> (ø)
...ns/LessonResourceSelectionPage/ContentCardList.vue 0% <0%> (ø)
...urceSelectionPage/LessonContentCard/CornerIcon.vue 0% <0%> (ø)
... and 36 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 bbc6266...2192061. Read the comment docs.

@codecov
Copy link

codecov bot commented Sep 18, 2018

Codecov Report

Merging #4253 into develop will increase coverage by 0.01%.
The diff coverage is 20.58%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4253      +/-   ##
===========================================
+ Coverage     53.2%   53.21%   +0.01%     
===========================================
  Files          693      699       +6     
  Lines        22046    22255     +209     
  Branches      2985     3024      +39     
===========================================
+ Hits         11729    11843     +114     
- Misses        9622     9710      +88     
- Partials       695      702       +7
Impacted Files Coverage Δ
...assets/src/views/assignments/AssignmentSummary.vue 20% <ø> (ø) ⬆️
...ch/assets/src/views/exams/CreateExamPage/index.vue 0% <ø> (ø) ⬆️
...s/src/views/assignments/AssignmentDetailsModal.vue 82.92% <ø> (ø) ⬆️
...coach/assets/src/views/lessons/LessonsRootPage.vue 0% <ø> (ø) ⬆️
...nResourceSelectionPage/LessonContentCard/index.vue 0% <ø> (ø) ⬆️
...ch/assets/src/views/exams/CoachExamsPage/index.vue 0% <ø> (ø) ⬆️
...ach/assets/src/modules/lessonResources/handlers.js 0% <0%> (ø) ⬆️
...nPage/SearchTools/ResourceSelectionBreadcrumbs.vue 0% <0%> (ø)
...ns/LessonResourceSelectionPage/ContentCardList.vue 0% <0%> (ø)
...urceSelectionPage/LessonContentCard/CornerIcon.vue 0% <0%> (ø)
... and 36 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 bbc6266...2192061. Read the comment docs.

@codecov
Copy link

codecov bot commented Sep 18, 2018

Codecov Report

Merging #4253 into develop will increase coverage by 0.01%.
The diff coverage is 20.58%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4253      +/-   ##
===========================================
+ Coverage     53.2%   53.21%   +0.01%     
===========================================
  Files          693      699       +6     
  Lines        22046    22255     +209     
  Branches      2985     3024      +39     
===========================================
+ Hits         11729    11843     +114     
- Misses        9622     9710      +88     
- Partials       695      702       +7
Impacted Files Coverage Δ
...assets/src/views/assignments/AssignmentSummary.vue 20% <ø> (ø) ⬆️
...ch/assets/src/views/exams/CreateExamPage/index.vue 0% <ø> (ø) ⬆️
...s/src/views/assignments/AssignmentDetailsModal.vue 82.92% <ø> (ø) ⬆️
...coach/assets/src/views/lessons/LessonsRootPage.vue 0% <ø> (ø) ⬆️
...nResourceSelectionPage/LessonContentCard/index.vue 0% <ø> (ø) ⬆️
...ch/assets/src/views/exams/CoachExamsPage/index.vue 0% <ø> (ø) ⬆️
...ach/assets/src/modules/lessonResources/handlers.js 0% <0%> (ø) ⬆️
...nPage/SearchTools/ResourceSelectionBreadcrumbs.vue 0% <0%> (ø)
...ns/LessonResourceSelectionPage/ContentCardList.vue 0% <0%> (ø)
...urceSelectionPage/LessonContentCard/CornerIcon.vue 0% <0%> (ø)
... and 36 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 bbc6266...2192061. Read the comment docs.

@codecov
Copy link

codecov bot commented Sep 18, 2018

Codecov Report

Merging #4253 into develop will increase coverage by 0.01%.
The diff coverage is 20.58%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4253      +/-   ##
===========================================
+ Coverage     53.2%   53.21%   +0.01%     
===========================================
  Files          693      699       +6     
  Lines        22046    22255     +209     
  Branches      2985     3024      +39     
===========================================
+ Hits         11729    11843     +114     
- Misses        9622     9710      +88     
- Partials       695      702       +7
Impacted Files Coverage Δ
...assets/src/views/assignments/AssignmentSummary.vue 20% <ø> (ø) ⬆️
...ch/assets/src/views/exams/CreateExamPage/index.vue 0% <ø> (ø) ⬆️
...s/src/views/assignments/AssignmentDetailsModal.vue 82.92% <ø> (ø) ⬆️
...coach/assets/src/views/lessons/LessonsRootPage.vue 0% <ø> (ø) ⬆️
...nResourceSelectionPage/LessonContentCard/index.vue 0% <ø> (ø) ⬆️
...ch/assets/src/views/exams/CoachExamsPage/index.vue 0% <ø> (ø) ⬆️
...ach/assets/src/modules/lessonResources/handlers.js 0% <0%> (ø) ⬆️
...nPage/SearchTools/ResourceSelectionBreadcrumbs.vue 0% <0%> (ø)
...ns/LessonResourceSelectionPage/ContentCardList.vue 0% <0%> (ø)
...urceSelectionPage/LessonContentCard/CornerIcon.vue 0% <0%> (ø)
... and 36 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 bbc6266...2192061. Read the comment docs.

@codecov
Copy link

codecov bot commented Sep 18, 2018

Codecov Report

❗ No coverage uploaded for pull request base (develop@78bbe07). Click here to learn what that means.
The diff coverage is 19%.

Impacted file tree graph

@@            Coverage Diff             @@
##             develop    #4253   +/-   ##
==========================================
  Coverage           ?   53.34%           
==========================================
  Files              ?      702           
  Lines              ?    22423           
  Branches           ?     3053           
==========================================
  Hits               ?    11962           
  Misses             ?     9739           
  Partials           ?      722
Impacted Files Coverage Δ
...sets/src/views/lessons/LessonSummaryPage/index.vue 0% <ø> (ø)
...assets/src/views/assignments/AssignmentSummary.vue 20% <ø> (ø)
...ch/assets/src/views/exams/CreateExamPage/index.vue 0% <ø> (ø)
...libri/plugins/learn/assets/src/views/SearchBox.vue 14.81% <ø> (ø)
...s/src/views/assignments/AssignmentDetailsModal.vue 82.92% <ø> (ø)
...coach/assets/src/views/lessons/LessonsRootPage.vue 0% <ø> (ø)
...nResourceSelectionPage/LessonContentCard/index.vue 0% <ø> (ø)
...lessons/LessonContentPreviewPage/SelectOptions.vue 0% <ø> (ø)
...ch/assets/src/views/exams/CoachExamsPage/index.vue 0% <ø> (ø)
...ach/assets/src/modules/lessonResources/handlers.js 0% <0%> (ø)
... and 11 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 78bbe07...bef1823. Read the comment docs.

@indirectlylit indirectlylit added the TAG: user strings Application text that gets translated label Sep 19, 2018
Copy link
Contributor

@indirectlylit indirectlylit left a comment

Choose a reason for hiding this comment

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

Notes

This button should say 'Manage resources':

image

'Total resources selected: 2' is confusing with auto-save. Recommend changing to '2 resources in lesson' or similar

image

Similarly here:

image

Instead of 'Undo', recommend this link says 'Remove' because it could have happened a long time ago

image

// Map of { kind, channel, role }
type: Object,
required: true,
},

This comment was marked as spam.

getParams: {
search: params.searchTerm,
},
}).then(results => {

This comment was marked as spam.

},
}).then(results => {
return showResourceSelectionPage(store, {
classId: params.classId,

This comment was marked as spam.

indirectlylit
indirectlylit previously approved these changes Sep 19, 2018
Copy link
Contributor

@indirectlylit indirectlylit left a comment

Choose a reason for hiding this comment

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

seems to work well. pushed some updates addressing feedback so we can get it merged in for string freeze

@indirectlylit indirectlylit dismissed their stale review September 19, 2018 23:17

dismissing review until pagination changes are addressed

Copy link
Contributor

@indirectlylit indirectlylit left a comment

Choose a reason for hiding this comment

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

creating follow-up issue #4297 - @rtibbles feel free to add notes there

@indirectlylit indirectlylit merged commit a7a125b into learningequality:develop Sep 20, 2018
@jonboiser jonboiser deleted the lessons-updates branch October 10, 2018 22:27
@indirectlylit indirectlylit added changelog Important user-facing changes and removed TODO: needs review Waiting for review labels Oct 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog Important user-facing changes TAG: user strings Application text that gets translated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants