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

Allow editing of lesson group fields in script editor gui #36968

Merged
merged 40 commits into from Oct 8, 2020

Conversation

dmcavoy
Copy link
Contributor

@dmcavoy dmcavoy commented Sep 29, 2020

Adds the ability to edit the Lesson Group name, description and big questions on the LessonGroupCard in the Script Editor GUI. The input/textareas only show if the Lesson Group is user facing. Otherwise it shows that you need to make the LessonGroup user facing and provides a button to do so.

Not User Facing

Screen Shot 2020-09-29 at 2 39 16 PM

User Facing

Screen Shot 2020-09-29 at 2 39 30 PM

Links

Testing story

Reviewer Checklist:

  • Tests provide adequate coverage
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

@dmcavoy dmcavoy changed the base branch from staging to new-editor-components October 1, 2020 20:46
Base automatically changed from new-editor-components to staging October 2, 2020 18:51
Copy link
Member

@davidsbailey davidsbailey left a comment

Choose a reason for hiding this comment

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

The UX seems like it might be a bit confusing regarding "user facing". this implementation mirrors what's in the database, but I think we could probably offer a simpler mental model to the editor.

In the first screenshot, when there is only one non-user-facing lesson group, what do you think about taking away the "user facing" button on the lesson group, and replacing the "add lesson group" button on the Unit card with a button named "enable lesson groups"?

In this state, you could also omit this header row:
Screen Shot 2020-10-02 at 11 40 48 AM

usual caveat: I am not a UX designer. :-) any thoughts? any input, @tess323 ?

lessonGroup: {
key: 'lg-key',
display_name: 'Display Name',
position: 1,
user_facing: true,
description: 'Lesson group description',
big_questions: '',
Copy link
Member

Choose a reason for hiding this comment

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

now seems like a good time to add a test showing that these two fields appear in the shallow render.

@dmcavoy
Copy link
Contributor Author

dmcavoy commented Oct 2, 2020

The UX seems like it might be a bit confusing regarding "user facing". this implementation mirrors what's in the database, but I think we could probably offer a simpler mental model to the editor.

In the first screenshot, when there is only one non-user-facing lesson group, what do you think about taking away the "user facing" button on the lesson group, and replacing the "add lesson group" button on the Unit card with a button named "enable lesson groups"?

In this state, you could also omit this header row:
Screen Shot 2020-10-02 at 11 40 48 AM

usual caveat: I am not a UX designer. :-) any thoughts? any input, @tess323 ?

I talked to Tess and she agreed with your thoughts on the design. We also talked about preventing you from ever having 0 lesson groups. Instead if you are starting a new script or delete all the existing lesson groups we will have a non-user facing lesson group already provided for you. I'm going to work on these changes and I will update the PR.

@dmcavoy
Copy link
Contributor Author

dmcavoy commented Oct 2, 2020

@tess323 @davidsbailey Updated!

user-facing-lesson-group

@davidsbailey
Copy link
Member

nice improvements! top-level comment: I would suggest hiding the up / down / delete controls in the case where lesson groups are not enabled:
Screen Shot 2020-10-05 at 2 06 18 PM

Copy link
Member

@davidsbailey davidsbailey left a comment

Choose a reason for hiding this comment

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

Nice progress Dani! A few more comments.

Comment on lines +81 to +82
// Remove lesson group when there is only one lesson group left
// a non-user facing lesson group should be added
Copy link
Member

Choose a reason for hiding this comment

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

If I am understanding correctly then I'm a bit worried about this user flow - if i enable lesson groups, then realize I've made a mistake, it seems much more likely that I'll want to convert the one remaining lesson group back to non-user-facing, as opposed to deleting all my lessons. I don't have an exact solution in mind, but accidentally clicking "enable" seems like a bit of a land mine for curriculum writers.

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'll look at how hard it is to have a disable lesson groups button if there is only one lesson group

Copy link
Member

Choose a reason for hiding this comment

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

ok, this would be fine to resolve in a future PR / discussion if that's easier.

apps/src/sites/studio/pages/scripts/edit.js Outdated Show resolved Hide resolved
Comment on lines +81 to +82
// Remove lesson group when there is only one lesson group left
// a non-user facing lesson group should be added
Copy link
Member

Choose a reason for hiding this comment

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

ok, this would be fine to resolve in a future PR / discussion if that's easier.

@dmcavoy
Copy link
Contributor Author

dmcavoy commented Oct 8, 2020

@davidsbailey @tess323 How does this flow look?

user-facing-lesson-group

@davidsbailey
Copy link
Member

❤️ 🚀 👍 awesome work Dani! This seals off all the problem cases I was worried about.

@dmcavoy dmcavoy merged commit 91c89e9 into staging Oct 8, 2020
@dmcavoy dmcavoy deleted the lesson-group-fields branch October 8, 2020 19:54
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.

None yet

2 participants