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 Facilitator Notes #2927

Open
wants to merge 25 commits into
base: trunk
Choose a base branch
from
Open

Add Facilitator Notes #2927

wants to merge 25 commits into from

Conversation

renintw
Copy link
Contributor

@renintw renintw commented Oct 7, 2024

Resolves #2869

This PR introduces the UI for Facilitator Notes, including updates for mobile view. Additionally, the Breadcrumbs section has been changed from "Home" to "Learn WordPress."

Screencast

facilitator.mp4

Lesson Plan Migration
From the discussion at #2392, it appears that before migrating Lesson Plans to Facilitator Notes, it needs to map each Lesson Plan to the most appropriate lesson. Has there been any related discussion?

@renintw renintw changed the title 2855 add facilitator notes Add Facilitator Notes Oct 7, 2024
@renintw renintw force-pushed the 2855-add-facilitator-notes branch 5 times, most recently from ffafa51 to eb899e9 Compare October 14, 2024 22:25
@renintw renintw self-assigned this Oct 14, 2024
@renintw renintw marked this pull request as ready for review October 14, 2024 22:39
@renintw renintw added [Component] Learn Theme Website development issues related to the Learn theme. [Component] Sensei Website development issues related to the Sensei plugin installed on Learn. labels Oct 14, 2024
@renintw renintw added this to the Learning Pathways Post-launch milestone Oct 14, 2024
@fcoveram
Copy link

The flow looks good to me. I'd love to hear @jonathanbossenger and @kathrynwp's thoughts.

I would also like to see if we can test this view with actual use cases. This post was shared before as an example of how long content could be, but it's vital to identify whether that page is an exception or not.

It would be frustrating to have a long modal, but also a poor user experience needing to open an external link to see short content that could perfectly fit into a modal.

@adamwoodnz
Copy link
Contributor

Lesson Plan Migration
From the discussion at #2392, it appears that before migrating Lesson Plans to Facilitator Notes, it needs to map each Lesson Plan to the most appropriate lesson. Has there been any related discussion?

No further discussion that I'm aware of. I'm imagining a new post meta field on a Lesson for selecting a Lesson Plan. The content from that Lesson Plan is rendered in the modal.

Copy link
Contributor

Choose a reason for hiding this comment

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

We'll need to apply these changes to patterns/sensei-lesson-standalone.php too

}
}

.wporg-learn-facilitator-notes-content {
Copy link
Contributor

@adamwoodnz adamwoodnz Oct 16, 2024

Choose a reason for hiding this comment

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

I notice that the modal content overflow is hidden. Some of the Lesson Plan content is very long so this will need to be scrollable on desktop as well as mobile.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. c125bcb


?>

<input type="checkbox" id="wporg-learn-facilitator-notes-toggle">
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to consider the accessibility of this control and the modal content. At present this input isn't focusable with keyboard. We may want to move focus to the close button after activation. I'd consult some modal best practice docs.

On mobile it is definitely following a modal pattern, where focus should be trapped within the modal, and background page elements should not be interactive. On desktop it's arguable that the rest of the UI should remain interactive, as it's more of a panel.

@ryelle any thoughts on this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also I expected this pattern would use a button element, hooked up to the interactivity API to control the modal visibility, however I'm not sure whether this input based approach is any less accessible. Might need more aria attributes though as I think it's a less typical pattern.

Copy link
Contributor

Choose a reason for hiding this comment

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

I expected this pattern would use a button element,

I think a button (with aria-expanded and aria-controls attributes) would be better. Right now this would read as checking a checkbox, with no way to know that it would reveal more content.

On mobile it is definitely following a modal pattern, where focus should be trapped within the modal, and background page elements should not be interactive. On desktop it's arguable that the rest of the UI should remain interactive, as it's more of a panel.

If it's easier, you can make it a modal (with focus trapping) in both cases - and if someone clicks outside the panel to use the rest of the site, close the panel. Otherwise, on desktop, I would expect it to close when I tab through everything, and focus moves on to the next tab stop (from the screenshots, I assume that's "exit course"). The desktop global nav submenus work like this— if you open "Community" & start tabbing, the submenu closes when "About" is focused.

left: 4%;
width: 92%;
height: 90vh;
border: 1px solid var(--sensei-course-progress-bar-color);
Copy link
Contributor

Choose a reason for hiding this comment

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

Seeing a grey border around the modal on mobile, I don't think this matches modals on other sites, eg. Meeting Calendar

Facilitator notes Facilitator notes inspected Meeting Calendar
Screenshot 2024-10-17 at 11 36 15 AM Screenshot 2024-10-17 at 11 36 24 AM Screenshot 2024-10-17 at 11 34 56 AM

Copy link
Contributor

@adamwoodnz adamwoodnz left a comment

Choose a reason for hiding this comment

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

Nice start! Visually the UI is working well. I've left some comments about accessibility concerns.

If we want to load the content from a linked Lesson Plan I guess we'll need a block with conditional rendering to handle the Lesson Plan being defined or erroring, etc.

@jonathanbossenger
Copy link
Collaborator

jonathanbossenger commented Oct 17, 2024

I would also like to see if we can test this view with actual use cases. This post was shared before as an example of how long content could be, but it's vital to identify whether that page is an exception or not.

It would be frustrating to have a long modal, but also a poor user experience needing to open an external link to see short content that could perfectly fit into a modal.

I understand your concerns. I've selected a sampling of current lesson plans from the lesson plan landing page

Everything up to the "Example Lesson" section would need to be displayed in the lesson notes module, and as you can see in all three examples, even the shortest one (Upload a theme) is pretty detailed.

@fcoveram
Copy link

fcoveram commented Oct 17, 2024

Thanks @jonathanbossenger for adding more examples.

I think we should revisit this decision and, instead of showing the content in a modal, make the "Facilitator notes" link to open a new page.

Although the practice is not recommended (G200), I would suggest opening a new tab. Filling out forms or (quote) "prevent the loss of any form data that has already been entered" are use cases excluded from this requirement.

What do you think of this? Perhaps @ryelle knows more about it.

@ryelle
Copy link
Contributor

ryelle commented Oct 17, 2024

I think we should revisit this decision and, instead of showing the content in a modal, make the "Facilitator notes" link to open a new page.

Although the practice is not recommended (G200), I would suggest opening a new tab. Filling out forms or (quote) "prevent the loss of any form data that has already been entered" are use cases excluded from this requirement.

Is there progress that a user can loose? Lessons are mostly articles, as I recall from past testing. Clicking a link and then using the back button wouldn't cause "lose of data" when reading an article. Would this show up on a quiz, or are there many interactive lessons?

If there are, and this would be a "lose of data" case, the G200 technique also recommends giving advance warning that this will be a new tab (typically this is done with the ↗️ icon, but we're already using that for external links). The indicator would need to be visible and screen-reader-accessible.

Otherwise, I think it would be fine to keep it as a new page and let users open it as they choose.

@renintw
Copy link
Contributor Author

renintw commented Oct 21, 2024

If we want to load the content from a linked Lesson Plan I guess we'll need a block with conditional rendering to handle the Lesson Plan being defined or erroring, etc.

I originally thought of keeping it simple and achieving it with CSS. If the content can't be retrieved, it could be handled within the pattern and display the corresponding notice/warning. That said, the requirements have changed now, I'll think about how to make adjustments for a better approach.

@adamwoodnz
Copy link
Contributor

adamwoodnz commented Oct 28, 2024

Is there progress that a user can loose? Lessons are mostly articles, as I recall from past testing. Clicking a link and then using the back button wouldn't cause "lose of data" when reading an article. Would this show up on a quiz, or are there many interactive lessons?

I don't believe there is any state to lose, and Lessons Plans don't relate to any quizzes afaik. I feel like a new tab/window seems like a good approach, with sufficient warning, so that the plan can be referenced alongside the lesson. That seems to be the intention.

@kaitohm
Copy link
Contributor

kaitohm commented Oct 29, 2024

I feel like a new tab/window seems like a good approach, with sufficient warning, so that the plan can be referenced alongside the lesson. That seems to be the intention.

This front-end implementation of a new tab would work. The team's request is more around the back-end implementation. quoting #2392 (comment)

The desired end state [is]... The information of the lesson plan would be stored in a custom field / block within the lesson editor.

This is in order to reduce the need for us to have to edit duplicative information across different post types, and instead manage all information in one lesson. This also will help us with streamlining translation work.

Would this still be possible?

@adamwoodnz
Copy link
Contributor

adamwoodnz commented Oct 29, 2024

The desired end state [is]... The information of the lesson plan would be stored in a custom field / block within the lesson editor.
This is in order to reduce the need for us to have to edit duplicative information across different post types, and instead manage all information in one lesson. This also will help us with streamlining translation work.

Would this still be possible?

Thanks for reminding us of that requirement. While I'm not sure it will help streamline translation work (that might depend on which translation plugin we choose and how well it plays with custom fields), the ease of editing in one place is not to be overlooked.

I'm not sure how well a custom fields approach will handle content of this scale either. This seems to be the main challenge for both frontend and backend. Might need some testing.

A custom Facilitators Notes block within the post editor might be worth trying too.

Both these approaches will very likely require manual migration I expect.

@kaitohm
Copy link
Contributor

kaitohm commented Oct 29, 2024

Both these approaches will very likely require manual migration I expect.

The team will be fine with that 👍 I believe we expected that would be the case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Component] Learn Theme Website development issues related to the Learn theme. [Component] Sensei Website development issues related to the Sensei plugin installed on Learn.
Projects
Status: 👀 In review (PRs only)
Development

Successfully merging this pull request may close these issues.

A revamped design in Lesson flow
6 participants