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

feat: add library component sidebar [FC-0062] #1217

Merged

Conversation

rpenido
Copy link
Contributor

@rpenido rpenido commented Aug 15, 2024

Description

This PR adds the library components sidebar

image

More information

Testing instructions

  • Open a library in the course authoring MFE
  • Click on a component card in the library home
  • Check if the library component sidebar opens
  • Edit the component name using the pencil icon
  • Check the result

Private ref: FAL-3800

Settings

EDX_PLATFORM_REPOSITORY: https://github.com/open-craft/edx-platform.git
EDX_PLATFORM_VERSION: rpenido/fal-3800-allow-update-fields-without-data

TUTOR_GROVE_ADDITIONAL_DOMAINS:
 - domain: meilisearch.{{ LMS_HOST }}

TUTOR_GROVE_MFE_LMS_COMMON_SETTINGS: MFE_CONFIG["LIBRARY_MODE"] = "mixed"

PLUGINS:
- mfe
- grove
- s3
- meilisearch

Tutor requirements

git+https://github.com/overhangio/tutor.git@nightly
git+https://github.com/overhangio/tutor-mfe@nightly
git+https://gitlab.com/opencraft/dev/tutor-contrib-grove.git@main
git+https://github.com/open-craft/tutor-contrib-meilisearch.git@main
git+https://github.com/hastexo/[email protected]

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Aug 15, 2024
@openedx-webhooks
Copy link

openedx-webhooks commented Aug 15, 2024

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.

@rpenido rpenido force-pushed the rpenido/fal-3800-library-component-sidebar branch 5 times, most recently from 6d314f9 to bdfb949 Compare August 15, 2024 21:38
Copy link

codecov bot commented Aug 15, 2024

Codecov Report

Attention: Patch coverage is 95.96774% with 5 lines in your changes missing coverage. Please review.

Project coverage is 93.08%. Comparing base (64ffadd) to head (68ac667).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
src/library-authoring/components/ComponentCard.tsx 62.50% 2 Missing and 1 partial ⚠️
src/library-authoring/common/context.tsx 93.75% 1 Missing ⚠️
src/library-authoring/data/apiHooks.ts 96.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1217      +/-   ##
==========================================
+ Coverage   93.06%   93.08%   +0.02%     
==========================================
  Files         756      760       +4     
  Lines       13777    13890     +113     
  Branches     2984     3007      +23     
==========================================
+ Hits        12822    12930     +108     
- Misses        919      923       +4     
- Partials       36       37       +1     

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

@rpenido rpenido force-pushed the rpenido/fal-3800-library-component-sidebar branch 8 times, most recently from 52365b0 to ec72c02 Compare August 16, 2024 14:17
@rpenido rpenido added the create-sandbox open-craft-grove should create a sandbox environment from this PR label Aug 16, 2024
@rpenido rpenido force-pushed the rpenido/fal-3800-library-component-sidebar branch from ec72c02 to dfb46e4 Compare August 16, 2024 16:29
@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@rpenido rpenido force-pushed the rpenido/fal-3800-library-component-sidebar branch 2 times, most recently from ac61c9a to fe2295c Compare August 16, 2024 19:24
@open-craft-grove
Copy link

Sandbox deployment failed 💥
Please check the settings and requirements.
Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.
📜 Failure Logs
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@open-craft-grove
Copy link

Sandbox deployment failed 💥
Please check the settings and requirements.
Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.
📜 Failure Logs
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@open-craft-grove
Copy link

Sandbox deployment failed 💥
Please check the settings and requirements.
Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.
📜 Failure Logs
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@open-craft-grove
Copy link

Sandbox deployment failed 💥
Please check the settings and requirements.
Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.
📜 Failure Logs
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@open-craft-grove
Copy link

Sandbox deployment failed 💥
Please check the settings and requirements.
Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.
📜 Failure Logs
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@open-craft-grove
Copy link

Sandbox deployment failed 💥
Please check the settings and requirements.
Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.
📜 Failure Logs
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@open-craft-grove
Copy link

Sandbox deployment failed 💥
Please check the settings and requirements.
Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.
📜 Failure Logs
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@open-craft-grove
Copy link

Sandbox deployment failed 💥
Please check the settings and requirements.
Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.
📜 Failure Logs
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@bradenmacdonald
Copy link
Contributor

@rpenido OK, something weird is going on here.

Before:
Screenshot 2024-08-26 at 10 12 58 AM

After:
Screenshot 2024-08-26 at 10 13 34 AM

  1. Did you intentionally change the width of the "Library Info Sidebar"? Now it seems too wide.
  2. Did you make the sidebar so it no longer overlaps with the header? That may be OK but we should clear it with the UX team if it wasn't their idea. The way that the "user widget" in the header is now randomly aligned with the sidebar based on the user's resolution looks bad in my opinion. CC @lizc577 @sdaitzman Screenshot 2024-08-26 at 10 14 48 AM
  3. The white background on the sidebar is nice though.

Screenshot 2024-08-26 at 10 17 06 AM

  1. ^ On a large monitor, the sidebar is too big and the buttons look terrible. We should have a fixed width for the sidebar above a certain size. It shouldn't get this wide, I think.
  2. I think the buttons should always stretch to fill the full width, not have these gaps between them: Screenshot 2024-08-26 at 10 18 57 AM

Screenshot 2024-08-26 at 10 19 44 AM

  1. On small screens, or when the text is translated into another language (e.g. Russian) where the description is longer, the buttons currently get "fat" (see above). I think it would be better to wrap them like this: Screenshot 2024-08-26 at 10 23 52 AM

But let's see what @lizc577 @sdaitzman say.

@rpenido rpenido force-pushed the rpenido/fal-3800-library-component-sidebar branch from 04f3a80 to 3c7007e Compare August 26, 2024 18:14
@rpenido
Copy link
Contributor Author

rpenido commented Aug 26, 2024

Did you intentionally change the width of the "Library Info Sidebar"? Now it seems too wide.

Yes.. I was trying to make it wider in the 1200-1320 range. I added a maximum width now to prevent it from going wider now: 3c7007e

Did you make the sidebar so it no longer overlaps with the header? That may be OK but we should clear it with the UX team if it wasn't their idea. The way that the "user widget" in the header is now randomly aligned with the sidebar based on the user's resolution looks bad in my opinion. CC @lizc577 @sdaitzman

Yes. I changed it to match the Figma design.
image
I didn't find a discussion about it, but I agree that the alignment looks odd (because header have a max xl size). Putting our container (content + sidebar) at a max width of xl will give us a better alignment, but we would lose a lot of screen space.

^ On a large monitor, the sidebar is too big and the buttons look terrible. We should have a fixed width for the sidebar above a certain size. It shouldn't get this wide, I think.

Added a max width 3c7007e

I think the buttons should always stretch to fill the full width, not have these gaps between them

Fixed

On small screens, or when the text is translated into another language (e.g. Russian) where the description is longer, the buttons currently get "fat" (see above). I think it would be better to wrap them like this

I also changed this

I still need to fix the responsiveness for small resolutions, but I will wait for the design team before tinkering with it.

sidebar-responsiveness

@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@sdaitzman
Copy link

@rpenido @bradenmacdonald thanks for checking in about these UX issues.

  1. Did you intentionally change the width of the "Library Info Sidebar"? Now it seems too wide.

In the screenshot, the sidebar looks far too wide. @lizc577 may have more specific constraints, but I think something around 1000px as a maximum size makes sense. Maybe even smaller?

  1. Did you make the sidebar so it no longer overlaps with the header? That may be OK but we should clear it with the UX team if it wasn't their idea. The way that the "user widget" in the header is now randomly aligned with the sidebar based on the user's resolution looks bad in my opinion.

Is it possible to make the header full width and vertically aligned with the rest of the page, e.g. place the right page header elements vertically in line with the right sidebar elements? This is an example from Figma with lots of placeholder elements, let me know if a more accurate example with the real header elements would be helpful.

image
  1. The white background on the sidebar is nice though.

Agreed. This is inverted from the designs in Figma, but it provides the intended slight visual separation.

  1. ^ On a large monitor, the sidebar is too big and the buttons look terrible. We should have a fixed width for the sidebar above a certain size. It shouldn't get this wide, I think.

Agreed, looks too wide to me. Thanks for fixing this @rpenido.

  1. I think the buttons should always stretch to fill the full width, not have these gaps between them:

Agreed, I think full width buttons make sense.

  1. On small screens, or when the text is translated into another language (e.g. Russian) where the description is longer, the buttons currently get "fat" (see above). I think it would be better to wrap them like this: Screenshot 2024-08-26 at 10 23 52 AM

That wrapping makes sense to me rather than showing multiple lines of text in one button, when possible to avoid that. The overflow menu ... icon position is a bit strange, but I think it's okay for now.

@bradenmacdonald
Copy link
Contributor

@rpenido That looks much better - thanks!

@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@ChrisChV
Copy link
Contributor

Thanks @rpenido the buttons look better.

  1. Did you intentionally change the width of the "Library Info Sidebar"? Now it seems too wide.

In the screenshot, the sidebar looks far too wide. @lizc577 may have more specific constraints, but I think something around 1000px as a maximum size makes sense. Maybe even smaller?

  1. Did you make the sidebar so it no longer overlaps with the header? That may be OK but we should clear it with the UX team if it wasn't their idea. The way that the "user widget" in the header is now randomly aligned with the sidebar based on the user's resolution looks bad in my opinion.

Is it possible to make the header full width and vertically aligned with the rest of the page, e.g. place the right page header elements vertically in line with the right sidebar elements? This is an example from Figma with lots of placeholder elements, let me know if a more accurate example with the real header elements would be helpful.

@rpenido I saw the same problems, Is there anything that can be done?

Captura desde 2024-08-26 18-12-33

@rpenido
Copy link
Contributor Author

rpenido commented Aug 27, 2024

In the screenshot, the sidebar looks far too wide. @lizc577 may have more specific constraints, but I think something around 1000px as a maximum size makes sense. Maybe even smaller?

Now I'm using the xs breakpoint (extra small) as maximum size, which is 464px (https://paragon-openedx.netlify.app/foundations/layout/#max-width). Let me know what you think!

Is it possible to make the header full width and vertically aligned with the rest of the page, e.g. place the right page header elements vertically in line with the right sidebar elements? This is an example from Figma with lots of placeholder elements, let me know if a more accurate example with the real header elements would be helpful.

It is possible, but we should try to be consistent throughout the app.

Currently, all pages (with some minor padding/margin issues 😅 ) have a maximum width in the xl breakpoint (1440px), the same size as the maximum header and footer.

Studio Home
image

Course Outline
image

Library Home (WITHOUT sidebar)
image

The Library Home (WITH sidebar) is currently breaking this rule, hence, it looks not aligned
image

Staying inside the xl breakpoint would make the whole app look more consistent.
We will have a sidebar that doesn't come from the page's edge.
image

@sdaitzman, is it possible to tweak the sidebar to make it look bet,ter following the max xl rule, like in the last screenshot?
I don't think it's the better solution, but should I keep it the way it was before (not wrapped by the header/footer)?

CC @ChrisChV @bradenmacdonald

@sdaitzman
Copy link

It is possible, but we should try to be consistent throughout the app.

Currently, all pages (with some minor padding/margin issues 😅 ) have a maximum width in the xl breakpoint (1440px), the same size as the maximum header and footer.

Thanks @rpenido! I can propose some sidebar design variants to avoid the staggered right vertical, but I would personally support slightly modifying the header within content libraries to fix the alignment issue. The header's contents would stay the same, just positioned to fill the screen and line up with the content libraries layout.

I think this potentially also interacts with #1221. Currently, components and collections are limited to four columns with wide gutters on large viewports. I think that users will benefit from using their full screen area when managing content libraries, and that it will look strange if the header is extremely narrow once the cards take up the full space.

@bradenmacdonald
Copy link
Contributor

@rpenido @sdaitzman Feel free to design whatever you'd like as a long-term solution, but I'd like to keep any changes to the header outside of the scope of our MVP. For now, having the sidebar as it already is on the sandbox (see below) avoids any issues - the header stays aligned with the content outside of the sidebar, and the sidebar has maximum real estate.

Screenshot 2024-08-27 at 2 22 50 PM

@rpenido
Copy link
Contributor Author

rpenido commented Aug 27, 2024

Changed the sidebar position and fixed responsiveness
sidebar-responsiveness-updated

@open-craft-grove
Copy link

Sandbox deployment failed 💥
Please check the settings and requirements.
Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.
📜 Failure Logs
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@bradenmacdonald
Copy link
Contributor

@rpenido That looks good to me - nice work! And thanks for the animation, that's helpful :)

@rpenido
Copy link
Contributor Author

rpenido commented Aug 28, 2024

@ChrisChV @sdaitzman Do you agree to move forward with the current version?

@open-craft-grove
Copy link

Sandbox deployment failed 💥
Please check the settings and requirements.
Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.
📜 Failure Logs
ℹ️ Grove Config, Tutor Config, Tutor Requirements

Copy link
Contributor

@ChrisChV ChrisChV left a comment

Choose a reason for hiding this comment

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

@rpenido Looks good!

@ChrisChV ChrisChV merged commit 48e0ec1 into openedx:master Aug 29, 2024
8 checks passed
@openedx-webhooks
Copy link

@rpenido 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

@ChrisChV ChrisChV deleted the rpenido/fal-3800-library-component-sidebar branch August 29, 2024 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
create-sandbox open-craft-grove should create a sandbox environment from this PR FC Relates to an Axim Funded Contribution project open-source-contribution PR author is not from Axim or 2U
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

8 participants