feat: allow full width content in library authoring [FC-0062] #1258

rpenido · 2024-09-05T21:51:51Z

Description

This PR allows the library authoring home to go full-width to better fit the content.

Before:

After:

Responsiveness:

More information

Part of:

Adjustments to tile responsiveness when sidebar is opened #1221

Depends on:

feat: migrates Container to TypeScript; Container without max-width on docs site [FC-0062] paragon#3216
feat: expose containerProps in StudioFooter [FC-0062] frontend-component-footer#463
feat: expose containerProps in StudioHeader [FC-0062] frontend-component-header#529

Testing instructions

Checkout this branch and run npm install --legacy-peer-deps to ignore the peer-deps conflicts and make sure you have the correct dependencies
If you experience Module not found webpack errors, it might be due to dependency cache issues. Run npm run build, and restart tutor.
Open the library authoring page for a library with some components
Resize the screen and check the responsiveness

Private ref: FAL-3820

openedx-webhooks · 2024-09-05T21:51:55Z

Thanks for the pull request, @rpenido!

What's next?

Please work through the following steps to get your changes ready for engineering review:

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
- This process (including the steps you'll need to take) is documented here.
If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

Dependencies

This PR must be merged before / after / at the same time as ...
Blockers

This PR is waiting for OEP-1234 to be accepted.
Timeline information

This PR must be merged by XX date because ...
Partner information

This is for a course on edx.org.
Supporting documentation
Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

🔘 Let us know that your PR is ready for review:

Who will review my changes?

This repository is currently maintained by @openedx/2u-tnl. Tag them in a comment and let them know that your changes are ready for review.

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

The size and impact of the changes that it introduces
The need for product review
Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

pomegranited

@rpenido I've only done a code review (npm is taking its sweet time 😄 ), but left some comments for you here and on the related PRs.

pomegranited · 2024-09-06T05:32:39Z

package.json

@@ -63,7 +63,7 @@
    "@openedx-plugins/course-app-wiki": "file:plugins/course-apps/wiki",
    "@openedx-plugins/course-app-xpert_unit_summary": "file:plugins/course-apps/xpert_unit_summary",
    "@openedx/frontend-plugin-framework": "^1.2.1",
-    "@openedx/paragon": "^22.5.1",
+    "@openedx/paragon": "git+ssh://github.com/open-craft/paragon#0e0a66a794fbb6ccf61f7afa9264e096f132cd1b",


This commit hash doesn't match openedx/paragon#3216 :

Suggested change

"@openedx/paragon": "git+ssh://github.com/open-craft/paragon#0e0a66a794fbb6ccf61f7afa9264e096f132cd1b",

"@openedx/paragon": "git+ssh://github.com/open-craft/paragon#4a206aec3227fe3f6b8e00d93035a4563b251d3a",

The problem here is that our build system is not building git dependencies. So, I created another branch (https://github.com/open-craft/paragon/tree/rpenido/dist/fal-3820-add-full-width-container-option) with the component built (with the dist folder) to link here. I don't know how we can handle it better.

Also, I think this is why we are having dependency conflicts. Locally, we solve it with --legacy-peer-deps, but I can't make the CI run.

pomegranited · 2024-09-06T05:37:33Z

package.json

@@ -47,8 +47,8 @@
    "@dnd-kit/sortable": "^8.0.0",
    "@dnd-kit/utilities": "^3.2.2",
    "@edx/brand": "npm:@openedx/brand-openedx@^1.2.3",
-    "@edx/frontend-component-footer": "^14.0.3",
-    "@edx/frontend-component-header": "^5.3.3",
+    "@edx/frontend-component-footer": "git+ssh://github.com/open-craft/frontend-component-footer#335ad6d0b09b868bced3f9988dd355666e54a50c",


This commit hash doesn't match openedx/frontend-component-footer#463:

Suggested change

"@edx/frontend-component-footer": "git+ssh://github.com/open-craft/frontend-component-footer#335ad6d0b09b868bced3f9988dd355666e54a50c",

"@edx/frontend-component-footer": "git+ssh://github.com/open-craft/frontend-component-footer#3b9230d7eadc12b4a9db01898b05e56303ae181b",

pomegranited · 2024-09-06T05:38:50Z

package.json

-    "@edx/frontend-component-footer": "^14.0.3",
-    "@edx/frontend-component-header": "^5.3.3",
+    "@edx/frontend-component-footer": "git+ssh://github.com/open-craft/frontend-component-footer#335ad6d0b09b868bced3f9988dd355666e54a50c",
+    "@edx/frontend-component-header": "git+ssh://github.com/open-craft/frontend-component-header#8aefc103cbe8ba59b1df96bf0aa3bce406b6d38e",


This commit hash doesn't match openedx/frontend-component-header#529:

Suggested change

"@edx/frontend-component-header": "git+ssh://github.com/open-craft/frontend-component-header#8aefc103cbe8ba59b1df96bf0aa3bce406b6d38e",

"@edx/frontend-component-header": "git+ssh://github.com/open-craft/frontend-component-header#6ede5ee13d89b3fb6e7da1ec0f2f2c588016a51b",

src/header/Header.tsx

src/library-authoring/components/LibraryComponents.tsx

src/library-authoring/components/LibraryComponents.scss

src/index.scss

pomegranited

👍 Works beautifully, thank you @rpenido !

I tested this on my tutor devstack using the latest commit hashes from the dependent PRs, and the PR test instructions.
I read through the code
~~I checked for accessibility issues~~ N/A
~~Includes documentation~~ N/A
~~User-facing strings are extracted for translation~~ N/A

ChrisChV · 2024-09-18T18:37:34Z

@rpenido I don't know if this is on the scope of this task, but when the description of a component is large, then it causes a disparity in sizes:

rpenido · 2024-09-19T00:06:51Z

Hi @ChrisChV! This is fixed now!

pomegranited

One nit and one suggestion (which can go to the CCs if you're unsure), but this is working well @rpenido . Thank you for taking the time to improve the frontend component repos too!

👍

I tested this on a library with many components. Checked that the card heights remain constant (with nit fixed), they occupy the whole screen, and are smoothly responsive as page with changes.
I read through the code
~~I checked for accessibility issues~~ N/A
~~Includes documentation~~ N/A
~~User-facing strings are extracted for translation~~ N/A

pomegranited · 2024-09-19T22:52:23Z

src/header/Header.tsx

@@ -69,8 +73,9 @@ const Header = ({
        mainMenuDropdowns={mainMenuDropdowns}
        outlineLink={outlineLink}
        searchButtonAction={meiliSearchEnabled ? openSearchModal : undefined}
+        containerProps={containerProps}


Do we actually want different pages/sections of this Authoring app to show a differently-sized header? This is probably a question for the CCs with oversight over the whole app, but I would think it's better to have these params here, and remove the containerProps from this Header class, so other pages don't mess with them.

Suggested change

containerProps={containerProps}

containerProps={{

size: undefined,

}}

Do we actually want different pages/sections of this Authoring app to show a differently-sized header?

The idea came from here: #1221 (comment)
And there is a previous discussion here: #1217 (comment)
Personally, I prefer consistency throughout the app. But we would definitely use the extra space in the library authoring, and maybe changing everything would be a too big step.

src/library-authoring/components/ComponentCard.scss

Co-authored-by: Jillian <[email protected]>

rpenido · 2024-10-01T18:14:41Z

src/search-manager/data/api.ts

@@ -246,7 +246,6 @@ export async function fetchSearchResults({
    highlightPreTag: HIGHLIGHT_PRE_TAG,
    highlightPostTag: HIGHLIGHT_POST_TAG,
    attributesToCrop: ['content'],
-    cropLength: 20,


I changed to the default (10) to reduce the text in the description. The design asked to limit to 3 lines, but we can only control word count here. We can fine-tune this in the AC test review.

codecov · 2024-10-01T18:22:48Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.95%. Comparing base (e9c10c7) to head (f08d932).
Report is 3 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1258      +/-   ##
==========================================
+ Coverage   92.79%   92.95%   +0.16%     
==========================================
  Files        1037     1037              
  Lines       19505    19508       +3     
  Branches     4130     4133       +3     
==========================================
+ Hits        18099    18134      +35     
+ Misses       1342     1309      -33     
- Partials       64       65       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pomegranited

👍 Works perfectly, thank you @rpenido !

I tested this using the PR test instructions
I read through the code
I checked for accessibility issues by using my keyboard to navigate
~~Includes documentation~~ N/A
~~User-facing strings are extracted for translation~~ N/A

…x#1258) * feat: allow full width content in library authoring * chore: update header and footer versions --------- Co-authored-by: Jillian <[email protected]>

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Sep 5, 2024

rpenido force-pushed the rpenido/fal-3820-allow-full-width-content branch from b6e58d4 to 491bc7a Compare September 5, 2024 23:53

rpenido changed the title ~~feat: allow full width content in library authoring~~ feat: allow full width content in library authoring [FC-0062] Sep 6, 2024

pomegranited suggested changes Sep 6, 2024

View reviewed changes

rpenido mentioned this pull request Sep 9, 2024

feat: collections tab [FC-0062] #1257

Merged

pomegranited approved these changes Sep 10, 2024

View reviewed changes

mphilbrick211 added the FC Relates to an Axim Funded Contribution project label Sep 17, 2024

rpenido force-pushed the rpenido/fal-3820-allow-full-width-content branch from d118adb to bb12eca Compare September 18, 2024 22:51

rpenido force-pushed the rpenido/fal-3820-allow-full-width-content branch from bb12eca to 95acaaf Compare September 19, 2024 20:56

feat: allow full width content in library authoring

99da9c7

rpenido force-pushed the rpenido/fal-3820-allow-full-width-content branch from 95acaaf to 99da9c7 Compare September 19, 2024 21:05

pomegranited approved these changes Sep 19, 2024

View reviewed changes

rpenido and others added 6 commits September 20, 2024 11:18

fix: typo on css

9252b6e

Co-authored-by: Jillian <[email protected]>

Merge branch 'master' into rpenido/fal-3820-allow-full-width-content

b2a4cbf

chore: update header and footer

d5e4f5f

fix: reduce cropLength to the default 10

cbc1661

fix: eslint

d50c109

fix: recently modified grid

f08d932

rpenido force-pushed the rpenido/fal-3820-allow-full-width-content branch from 65e34a2 to f08d932 Compare October 1, 2024 18:12

rpenido commented Oct 1, 2024

View reviewed changes

rpenido marked this pull request as ready for review October 1, 2024 18:16

rpenido requested a review from a team as a code owner October 1, 2024 18:16

pomegranited approved these changes Oct 2, 2024

View reviewed changes

ChrisChV approved these changes Oct 2, 2024

View reviewed changes

ChrisChV merged commit 0349188 into openedx:master Oct 2, 2024
7 checks passed

ChrisChV deleted the rpenido/fal-3820-allow-full-width-content branch October 2, 2024 11:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: allow full width content in library authoring [FC-0062] #1258

feat: allow full width content in library authoring [FC-0062] #1258

rpenido commented Sep 5, 2024 •

edited

Loading

openedx-webhooks commented Sep 5, 2024 •

edited

Loading

pomegranited left a comment

pomegranited Sep 6, 2024

rpenido Sep 6, 2024 •

edited

Loading

pomegranited Sep 6, 2024

pomegranited Sep 6, 2024

pomegranited left a comment

ChrisChV commented Sep 18, 2024

rpenido commented Sep 19, 2024

pomegranited left a comment

pomegranited Sep 19, 2024

rpenido Sep 20, 2024

rpenido Oct 1, 2024

codecov bot commented Oct 1, 2024 •

edited

Loading

pomegranited left a comment

	"@openedx/paragon": "git+ssh://github.com/open-craft/paragon#0e0a66a794fbb6ccf61f7afa9264e096f132cd1b",
	"@openedx/paragon": "git+ssh://github.com/open-craft/paragon#4a206aec3227fe3f6b8e00d93035a4563b251d3a",

	"@edx/frontend-component-footer": "git+ssh://github.com/open-craft/frontend-component-footer#335ad6d0b09b868bced3f9988dd355666e54a50c",
	"@edx/frontend-component-footer": "git+ssh://github.com/open-craft/frontend-component-footer#3b9230d7eadc12b4a9db01898b05e56303ae181b",

	"@edx/frontend-component-header": "git+ssh://github.com/open-craft/frontend-component-header#8aefc103cbe8ba59b1df96bf0aa3bce406b6d38e",
	"@edx/frontend-component-header": "git+ssh://github.com/open-craft/frontend-component-header#6ede5ee13d89b3fb6e7da1ec0f2f2c588016a51b",

-        containerProps={containerProps}
+        containerProps={{
+            size: undefined,
+        }}

feat: allow full width content in library authoring [FC-0062] #1258

feat: allow full width content in library authoring [FC-0062] #1258

Conversation

rpenido commented Sep 5, 2024 • edited Loading

Description

Before:

After:

Responsiveness:

More information

Testing instructions

openedx-webhooks commented Sep 5, 2024 • edited Loading

What's next?

🔘 Get product approval

🔘 Provide context

🔘 Get a green build

🔘 Let us know that your PR is ready for review:

Who will review my changes?

Where can I find more information?

When can I expect my changes to be merged?

pomegranited left a comment

Choose a reason for hiding this comment

pomegranited Sep 6, 2024

Choose a reason for hiding this comment

rpenido Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

pomegranited Sep 6, 2024

Choose a reason for hiding this comment

pomegranited Sep 6, 2024

Choose a reason for hiding this comment

pomegranited left a comment

Choose a reason for hiding this comment

ChrisChV commented Sep 18, 2024

rpenido commented Sep 19, 2024

pomegranited left a comment

Choose a reason for hiding this comment

pomegranited Sep 19, 2024

Choose a reason for hiding this comment

rpenido Sep 20, 2024

Choose a reason for hiding this comment

rpenido Oct 1, 2024

Choose a reason for hiding this comment

codecov bot commented Oct 1, 2024 • edited Loading

Codecov Report

pomegranited left a comment

Choose a reason for hiding this comment

rpenido commented Sep 5, 2024 •

edited

Loading

openedx-webhooks commented Sep 5, 2024 •

edited

Loading

rpenido Sep 6, 2024 •

edited

Loading

codecov bot commented Oct 1, 2024 •

edited

Loading