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

Move Instructed PL Sections to My PL page #57947

Merged
merged 32 commits into from Apr 17, 2024
Merged

Conversation

TurnerRiley
Copy link
Contributor

@TurnerRiley TurnerRiley commented Apr 10, 2024

Moves the Professional Learning sections the user is teaching from studio.code.org/home to studio.code.org/my-professional-learning.

Following the spec, this PR should:

  • Show the table of the PL sections the user is teaching
    • Show it only for facilitators (under the "My Facilitator Center" tab), universal instructors (under the "Instructors" tab), and peer reviewers (under the "Instructors" tab)
    • Show the "Create Section" component
  • Show the co-teacher invites banners for professional learning sections that were on the Teacher Dashboard
  • Remove the PL sections table from the Teacher Dashboard and replace with a short message with a redirect to the My PL page

Note about recreating the Owned Sections Table

Unfortunately, there are a lot of different table setups used across the Teacher Dashboard and the My PL page so it was not as simple as moving the OwnedSectionsTable component over because the styling was pretty off as the My PL page's styles, the imported styles from tableConstants.js, and the in-file styles within a styles object were all blending together. I figured taking the time to refactor OwnedSectionsTable to maintain its current styles on the Teacher Dashboard but have different functionality and styling on the My PL page would be scope creep. So, my solution was to make a similar OwnedPlSectionsTable that is the same content copied over but with anything unnecessary removed. I also consolidated the style updates for this in the tableConstants.js file so that it wasn't as confusing to track. If this seems like a concern I'm open to pushback and/or can make a ticket for building out this table following the way Kelby did with the SelfPacedProgressTable so that the styles can be better reused.

Demos

View of page by permission

Facilitator:
(note: this Facilitator also has a non-PL section to demonstrate that this table only shows PL sections (both active and archived))
Facilitator

Universal instructor:
universalinstructor

Peer reviewer:
peer_reviewer

Teacher (should not be able to see it):
Teacherview

Co-teacher invite

coteacherinvite

Teacher Dashboard message where table used to be

teacherdashboard

Links

Jira ticket: here
Spec: here
Figma: here

Testing story

Local testing and adding a Storybook for OwnedPlSectionsTable.
OwnedPlSections

@TurnerRiley TurnerRiley requested a review from a team as a code owner April 10, 2024 23:33
@TurnerRiley TurnerRiley changed the base branch from staging to remove-recent-courses-component-from-teacher-homepage April 12, 2024 17:03
Base automatically changed from remove-recent-courses-component-from-teacher-homepage to staging April 15, 2024 15:41
@TurnerRiley TurnerRiley requested a review from a team April 16, 2024 00:13
@dmcavoy
Copy link
Contributor

dmcavoy commented Apr 16, 2024

I'm so excited to see this coming too life! Yay!!

Nit: Can we update the tab names to what is on this page in figma: https://www.figma.com/file/sZ6d1uhJWFksJ6j3Fnmbnc/PL-Flow-Improvements?type=design&node-id=1958-8045&mode=design&t=x4CDcEPgC2GtTFe6-0

Professional Learning
Facilitator Center
Regional Partner Center
Workshop Organizer Center
Instructor Center

@dmcavoy dmcavoy requested review from a team and lfryemason and removed request for a team April 16, 2024 00:38
@TurnerRiley
Copy link
Contributor Author

I'm so excited to see this coming too life! Yay!!

Nit: Can we update the tab names to what is on this page in figma: https://www.figma.com/file/sZ6d1uhJWFksJ6j3Fnmbnc/PL-Flow-Improvements?type=design&node-id=1958-8045&mode=design&t=x4CDcEPgC2GtTFe6-0

Professional Learning Facilitator Center Regional Partner Center Workshop Organizer Center Instructor Center

Oh gotcha I was going off the spec tab names, I'll update these! Do we still want to do "Instructors" instead of "Instructor Center" like we discussed offline?

@TurnerRiley TurnerRiley removed the request for review from a team April 16, 2024 17:27
// value: 'myWorkshopOrganizerCenter',
// text: i18n.plLandingTabWorkshopOrganizerCenter(),
// },
// {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: should this one still be commented out?

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 think since we haven't set up any content yet for those tabs I've been leaving them commented out until they are useful. Otherwise Regional Partners and Workshop Organizers will have empty tabs

)}
{deeperLearningCourseData?.length >= 1 && (
{['myFacilitatorCenter', 'instructors'].includes(currentTab) && (
Copy link
Contributor

Choose a reason for hiding this comment

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

The facilitator tab should have more components at some point. While adding those components is outside the scope of this PR, it might be worth separating out this logic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes for sure, my plan once we started implementing more components was to simplify the logic to a series of {shouldRenderXComponent && (<XComponent/>)} where shouldRenderXComponent would contain the logic of checking if the user has the right permissions and any other qualifications


return isPlSections ? (
<OwnedPlSectionsTable
isPlSections={true}
Copy link
Contributor

Choose a reason for hiding this comment

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

is isPlSections needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No not anymore, I guess I just wasn't sure how much I should change in the scope of this PR for updating OwnedSections and OwnedSectionsTable. Since this change would remove the need for isPlSections in several files would it be okay if I did a followup cleanup task for it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cleanup work set up in this PR: #58075

Copy link
Contributor

Choose a reason for hiding this comment

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

But it's not a prop on OwnedPlSectionsTable is it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe to clarify, totally fine with cleaning it up on OwnedSectionsTable as a follow-up but I'd prefer it be removed here in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh gotcha! Yes you're super right, just committed a fix!

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for adding all of these tests!!

@@ -1,45 +1,16 @@
@no_mobile
Feature: Professional learning Sections

Scenario: Create new professional learning section as levelbuilder
Copy link
Contributor

Choose a reason for hiding this comment

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

Just so I'm clear, it looks like the test for "as a levelbuilder" was removed but creating PL sections is still covered by other tests in this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep! I removed it because this section was moved to the My PL page and levelbuilders won't have their own view to see this section there

Copy link
Contributor

@bethanyaconnor bethanyaconnor left a comment

Choose a reason for hiding this comment

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

🎉

@TurnerRiley TurnerRiley merged commit 771e1ea into staging Apr 17, 2024
2 checks passed
@TurnerRiley TurnerRiley deleted the setup-pl-sections-on-my-pl branch April 17, 2024 21:45
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

4 participants