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 note #3052

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

Add facilitator note #3052

wants to merge 26 commits into from

Conversation

renintw
Copy link
Contributor

@renintw renintw commented Dec 4, 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."

Screenshots

Screen.Capture.on.2024-12-10.at.17-32-12.mov
Desktop Mobile
Editor image -
Frontend image image
  1. Based on this comment, which mentions:

The desired end state for 'Teacher’s Notes' is a tab or popup sidebar section within a lesson that can host the details of a lesson plan for a facilitator. The information of the lesson plan would be stored in a custom field / block within the lesson editor.

A Lesson Facilitator Note block has been added, which can be used within the Lesson Editor to select the corresponding lesson plan. The content can be edited and saved directly within the editor.
After saving the post, a Facilitator Notes label will be displayed in the previously discussed location. Clicking on this label will open a new tab showing the lesson plan.
And this block can be added anywhere within the post without affecting the final render of the article.

  1. Based on feedback from the previously opened PR, the styles for standalone lessons have also been adjusted to align with the design.

@renintw renintw force-pushed the 2869-add-facilitator-note branch from 482c918 to 1e2d4b0 Compare December 5, 2024 13:26
@renintw renintw self-assigned this Dec 10, 2024
@renintw renintw marked this pull request as ready for review December 10, 2024 08:54
@renintw renintw mentioned this pull request Dec 10, 2024
@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 Dec 10, 2024
@renintw renintw added this to the Learning Pathways Post-launch milestone Dec 10, 2024
@renintw renintw requested a review from ryelle December 10, 2024 08:57
@renintw renintw changed the title 2869 add facilitator note Add facilitator note Dec 10, 2024
@fcoveram
Copy link

All looks good to me. The only remaining point is on opening the facilitator notes. @ryelle shared a clear response for my question about opening a new tab or not

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.

As per my understanding, lessons do not display content that could be loose, as in quizzes. Therefore, we should open the notes by refreshing the page, as with the default page browsing.

@jonathanbossenger
Copy link
Collaborator

Hi folks, just a note on the conversation between opening in a new tab vs refreshing the page.

As per my understanding, lessons do not display content that could be loose, as in quizzes. Therefore, we should open the notes by refreshing the page, as with the default page browsing.

I want to remind everyone of the purpose of the facilitator notes/lesson plans. These are typically used by teachers/educators/presenters, not for learners. While learners might decide to view this content, they are not the primary consumers of lesson plan content. (lesson != lesson plan)

As a teacher/educator/presenter at a meetup, I want to present a topic based on a lesson on learn.wordpress.org. I open the lesson, and I want to see the facilitator's notes that help me prepare for it. I would, therefore, want to be able to see both at the same time, either in the modal we originally suggested or in a separate tab. (My personal preference would be to drag the tabs side by side, but that's me.)

So, this is not about learners losing content/progress but about making it easier for facilitators to prepare. Opening this in a new tab would still be ideal.

@jonathanbossenger
Copy link
Collaborator

@renintw I've tested this on my local environment, and it all works as expected. One thing I did note is that on higher resolution screens (I'm currently on 2560 x 1440 DPIx2), the facilitator notes link appears in a weird position (screenshot below)

Lesson-1-–-Lesson-Learn-WordPress-org-12-10-2024_12_36_PM

@kaitohm as this link between lessons and lesson plans has been implemented as a block in the editor, we might want to decide if we want the block to be added at the top or the bottom of the lesson. I'd recommend we require it at the bottom.

@ryelle
Copy link
Contributor

ryelle commented Dec 10, 2024

As a teacher/educator/presenter at a meetup, I want to present a topic based on a lesson on learn.wordpress.org. I open the lesson, and I want to see the facilitator's notes that help me prepare for it. I would, therefore, want to be able to see both at the same time, either in the modal we originally suggested or in a separate tab. (My personal preference would be to drag the tabs side by side, but that's me.)

So, this is not about learners losing content/progress but about making it easier for facilitators to prepare. Opening this in a new tab would still be ideal.

It's not about preventing opening in a new tab, so users can still cmd+click or right-click to open in a new tab. The issue is that it's an accessibility anti-pattern to force opening in a new tab. The exception would be if there was data loss, which is why that conversation came up. Leaving the option to the user is the best approach.

@jonathanbossenger
Copy link
Collaborator

@ryelle ah, my apologies, I understand now. When I originally read the conversation around when it was decided to move away from a modal to the external link, I had understood it would open in a new tab by default. I do understand that it's an accessibility anti-pattern to force opening in a new tab. I wasn't aware of the exception around data loss, which is why I misread that discussion.

@kaitohm I know the facilitator notes as a modal was something that @Piyopiyo-Kitsune originally wanted, specifically to be able to see both at the same time. Do you foresee any issues with the facilitator notes opening in the current window, instead of a new tab, and folks rather choosing, through the use of cmd/ctrl + click or right click open in a new tab?

@kaitohm
Copy link
Contributor

kaitohm commented Dec 11, 2024

@renintw Thanks for working on this! It's exciting to see this moving forward.

@jonathanbossenger

I'd recommend we require it at the bottom.

I agree. We can require as such in the team documentation when that's updated.

Do you foresee any issues with the facilitator notes opening in the current window, instead of a new tab, and folks rather choosing, through the use of cmd/ctrl + click or right click open in a new tab?

Not really...? I think we could implement it that way, and then iterate further on if we get feedback.

To clarify, we're not using a modal to show this information? It would replace the content on the current page?

@fcoveram
Copy link

Thanks @ryelle for the clarification.

To clarify, we're not using a modal to show this information? It would replace the content on the current page?

Yes. We're moving away from using the modal, as suggested here (a closed ticket that implemented the modal), and the link will refresh the page. As pointed out here, we're not forcing users to open a new tab and they decide how to navigate.

@renintw
Copy link
Contributor Author

renintw commented Dec 11, 2024

@renintw I've tested this on my local environment, and it all works as expected. One thing I did note is that on higher resolution screens (I'm currently on 2560 x 1440 DPIx2), the facilitator notes link appears in a weird position (screenshot below)

Oh, I figured out the reason. It's because I inserted the facilitator notes after the course progress counter and assumed that non-standalone courses would always have a course progress counter. I overlooked the view seen by unenrolled students.

It’s been fixed.

Do you think we'd want to restrict who can see this label? @jonathanbossenger

--

Note: I’ve updated the link to open in the same tab instead of a new one.

@jonathanbossenger
Copy link
Collaborator

Do you think we'd want to restrict who can see this label? @jonathanbossenger

Hmm good question. The person who would use this would be someone who is preparing an in-person lesson somewhere, based on this content. They might be enrolled in the course, or they might not. Therefore I don't think we'd want to restrict who can see this label.

@renintw renintw force-pushed the 2869-add-facilitator-note branch from 458d442 to affb77e Compare December 11, 2024 12:57
Copy link
Contributor

@ryelle ryelle left a comment

Choose a reason for hiding this comment

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

A Lesson Facilitator Note block has been added, which can be used within the Lesson Editor to select the corresponding lesson plan. The content can be edited and saved directly within the editor.

It surprised me that this content is editable and saves back to the Lesson Plan. Once it's in the Lesson editor, the way it's framed, I expected it to disconnect from the original. Was it being directly editable a feature requirement?

Will there be a case where someone wants to add facilitator notes to a lesson, but a separate Lesson Plan doesn't exist?


const fetchLessonPlans = debounce( ( searchTerm ) => {
apiFetch( {
path: `/wp/v2/lesson-plan?search=${ encodeURIComponent( searchTerm ) }&per_page=10`,
Copy link
Contributor

Choose a reason for hiding this comment

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

I had a lot of trouble finding specific lesson plans in my local site - I was typing exact keywords in the lesson title, but nothing would show up. I think because this search is default WP search, so it's returning when the keywords match in title or content, but the combobox control only matches results on title.

So the API is limited to 10 items, and my search "block directory" returned more than 10 items, pushing the page I wanted out of the options. And nothing would show up when I typed, because "block directory" is not a match in any of the results I had:

  • Create a Basic Child Theme for Block Themes
  • WebP images in WordPress
  • Υποβολή Μοτίβων μπλοκ (Block Patterns) στον κατάλογο.
  • Πώς να χρησιμοποιήσετε και να συνεισφέρετε στον κατάλογο φωτογραφιών του WordPress αλλά και στο Openverse
  • Submitting Block Patterns to the Directory
  • Template Editor
  • How to Use and Contribute to the WordPress Photo Directory and Openverse
  • How to Build Low-Code Block Patterns
  • How to Create and Register a Block Pattern
  • Difference between Reusable Blocks, Block Pattern, Templates, Template Parts

Maybe increase the result number? Or see if you can adjust the search for this endpoint to only do title searches.

You could preload the ID/title pairs, and not use an API request for this at all. With 150 published lesson plans, it shouldn't be that slow. Or if you'd rather not do that on the server side, fire off an API request to get all the lesson plans when the block loads (instead of just if ( lessonPlanId )).

@renintw
Copy link
Contributor Author

renintw commented Dec 13, 2024

It surprised me that this content is editable and saves back to the Lesson Plan. Once it's in the Lesson editor, the way it's framed, I expected it to disconnect from the original. Was it being directly editable a feature requirement?

Will there be a case where someone wants to add facilitator notes to a lesson, but a separate Lesson Plan doesn't exist?

I inferred this from this comment and the comments from the PR. However after taking a closer look, it seems that the main intention is to avoid editing the same document in different places.

So I think I can also change it here so that editing is not possible, or even hide the content entirely. And instead, provide a link to edit or add a new lesson plan.

What do you think, does it provide any benefit to edit directly within the lesson? @jonathanbossenger @kaitohm

@kaitohm
Copy link
Contributor

kaitohm commented Dec 23, 2024

Just to pause a minute, what we're seeing proposed here is very different from the feature requirements Training had requested. To bring back this comment, we're looking for:

  1. A popup modal within the lesson to display facilitator notes
  2. These notes are stored and edited in the Lesson post type
  3. The Lesson Plan post type is removed from Learn

It seems none of these requests are being met with the current implementation suggestion. Could we clarify the rationale behind why each of these three requests are not being implemented? I'd like to make sure the Training team understands the reasons for course correction here before we sign off on the suggested implementation. @renintw @ryelle @fcoveram 🙂

(This comment was referenced as the reason for moving away from a modal. But the reason behind the decision isn't clear to me there, only that the decision to move away was made in that comment.)

@fcoveram
Copy link

I can speak for the first point. Let me recap how the design unrolled.

The first proposal explored the idea of displaying the notes in a sidebar toggled from a link in the page header. Then, the feedback received was about concerns over showing extensive content in a narrow area.

The second iteration shifted to show the notes in a popover and modal, but a personal confusion on the content extension did not suit the real use case attached in this comment.

From there, I suggested moving us away from the popover and modal approach and instead going with a full-page solution. In that comment, I also suggested opening the notes in a new tab, but since the accessibility criteria (G200) says to apply “only when necessary,” I asked for thoughts.

This comment elaborated on the “lose of data” scenario and a possible solution (adding a top-right icon) from the G200. However, the content does not belong to any of the two situations described in the accessibility technique.

With the above, the comments on this PR (from here) have been mainly about applying G200 and “not forcing” users to open a new tab. Instead, they browse as within their preferences. That includes indeed opening a new tab.

To me, the following description from the accessibility guidelines criteria summarizes the core idea of not forcing users to open tabs.

…In general, it is better not to open new windows and tabs since they can be disorienting for people, especially people who have difficulty perceiving visual content…

Given the above, I would follow the accessibility recommendation and not open the notes in new tab.

Looking forward to seeing the Training team's thoughts and who makes the decision, as this seems to be our current blocker.

@jonathanbossenger
Copy link
Collaborator

@fcoveram I wonder if it would be useful to discuss your concern about a long modal being frustrating.

Are you concerned that the users of facilitator notes would find it frustrating having to scroll for the content they need in the notes or something else?

@fcoveram
Copy link

Are you concerned that the users of facilitator notes would find it frustrating having to scroll for the content they need in the notes or something else?

The scroll interaction is not a problem by itself; it’s an accepted behavior on many devices.

If most facilitator notes are long and have rich text content that might be difficult to read in a narrow space, like headings and bullets that add indentation spaces, then I’m drawn to display them on a different page. You mentioned something similar in a previous comment.

I appreciate that the term facilitator notes makes it sound like it needs to display a list of short notes, but in fact the content of those notes are in some cases even more detailed than the actual lesson itself, depending on the lesson content and preparation requirements.

The context is also crucial. And it seems there is no need for having them side by side, as pointed out in this comment.

I want to remind everyone of the purpose of the facilitator notes/lesson plans. These are typically used by teachers/educators/presenters, not for learners. While learners might decide to view this content, they are not the primary consumers of lesson plan content. (lesson != lesson plan)

(Notes in bold were added by me)

@kaitohm
Copy link
Contributor

kaitohm commented Dec 24, 2024

Thank you for the detailed summary, @fcoveram , this really helps 😃 Have we considered a full-screen modal instead of a new page? Here's an example of what I'm talking about. https://www.w3schools.com/bootstrap5/tryit.asp?filename=trybs_modal_fullscreen&stacked=h (I've also seen modals like this with a zoom in/out animation on opening/closing.)

From a content maintenance point of view, the team would really like to get rid of the additional post type. Having lesson content and facilitator notes live in different locations requires additional manual tracking to link the two and update the other when one has been modified.

I'd say if we can store all information in a single post type, then the visual representation is less of an issue here. (Though, I think a modal of some sorts would provide a stronger sense of continuity between the content and facilitator notes.)

I've brought the Training Team's attention to the last few comments here so that the team is aware of the progress of this discussion: https://wordpress.slack.com/archives/C02RW657Q/p1735020507769879

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
5 participants