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 a fluid container to allow for limiting the maximum width of the content #276

Merged

Conversation

Lunyachek
Copy link
Contributor

@Lunyachek Lunyachek commented Feb 7, 2024

Our proposal is to add <div class="container-fluid"> inside the <main id="main-content"> block. This will allow limiting the maximum width of the content when necessary. Otherwise, the content will span the entire width of the page, as it would without this div.

Example screenshot width content max-width
Снимок экрана 2024-02-07 в 23 13 05

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

Thanks for the pull request, @Lunyachek! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

Copy link

codecov bot commented Feb 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.22%. Comparing base (ff97cc8) to head (6eeca09).
Report is 22 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #276   +/-   ##
=======================================
  Coverage   70.22%   70.22%           
=======================================
  Files          27       27           
  Lines         403      403           
  Branches       85       85           
=======================================
  Hits          283      283           
  Misses        119      119           
  Partials        1        1           

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

@mphilbrick211 mphilbrick211 requested a review from a team February 8, 2024 18:10
@mphilbrick211
Copy link

Hi @hurtstotouchfire and @openedx/2u-aperture - are you still reviewing / merging PRs? If so, this one is good-to-go. If not, please let me know and I can assign a Core Contributor.

@jsnwesson
Copy link
Contributor

Hi @mphilbrick211 , we are still reviewing and merging OSPRs! Our on-call person is responsible for looking over any OSPRs that have a needs maintainer attention label, and they either review/merge the PR if it's small enough (like this one) or create a ticket in our team's internal board to triage and prioritize for the upcoming sprints.

Reference: Aperture Repo Maintainership Process

@jsnwesson jsnwesson added the waiting for eng review PR is ready for review. Review and merge it, or suggest changes. label Feb 14, 2024
@jsnwesson
Copy link
Contributor

Hey @Lunyachek , I tested this out in my local dev environment, and I'm seeing a worse experience with removing the padding and adding container-fluid as a classname.

The first screenshot is what Program Records looks like with container-fluid
The second screenshot is what PR looks like without the <div className="container-fluid"> and pl-4 pr-4 reinserted at in <main>.

Screenshot 2024-02-14 at 4 08 27 PM Screenshot 2024-02-14 at 4 08 16 PM

@jsnwesson jsnwesson added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed waiting for eng review PR is ready for review. Review and merge it, or suggest changes. labels Feb 15, 2024
@Lunyachek
Copy link
Contributor Author

Hello @jsnwesson! Do you mean that the padding on the left side of the content is too small? Please review my branch again. I have reverted the classes pl-4 and pr-4 back. However, I left the container-fluid inside the main. This is because without it, we can't control the maximum width of the content
Снимок экрана 2024-02-23 в 14 15 24

@jsnwesson
Copy link
Contributor

@Lunyachek thanks for looking over it and adding back the padding. Everything else looks good to me.

@jsnwesson jsnwesson merged commit 5ba4445 into openedx:master Feb 23, 2024
7 checks passed
@openedx-webhooks
Copy link

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

@Lunyachek
Copy link
Contributor Author

@jsnwesson, great! Please also take a look at the backport #277
Thanks!

@Lunyachek Lunyachek deleted the lunyachek/feat/add-fluid-container-master branch February 23, 2024 23:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc.
Projects
Status: Done
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants