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

[APT-1585] Courses page #99

Merged
merged 5 commits into from
Dec 15, 2021
Merged

[APT-1585] Courses page #99

merged 5 commits into from
Dec 15, 2021

Conversation

ebeneliason
Copy link
Contributor

This is a solid start towards the dedicated courses page. I've chosen to include the page underneath /docs/guides as a .mdx file, which enable us to utilize built-in layouts and markdown on the page in conjunction with the Courses component.

@netlify
Copy link

netlify bot commented Dec 15, 2021

✔️ Deploy Preview for pensive-meitner-faaeee ready!

🔨 Explore the source changes: e8212b4

🔍 Inspect the deploy log: https://app.netlify.com/sites/pensive-meitner-faaeee/deploys/61ba26ac87e74e00083f4863

😎 Browse the preview: https://deploy-preview-99--pensive-meitner-faaeee.netlify.app

@ebeneliason ebeneliason changed the title [WIP] Courses page [APT-1585] Courses page Dec 15, 2021
@ebeneliason
Copy link
Contributor Author

This is now ready for review. Here's the direct link to the page.

Note that there are some minor discrepancies between this and the mockup in Figma. Most notably, the page width and margins aren't the same. That's an artifact of utilizing the built-in Docusaurus layout, which we want to do since it allows us to utilize markdown (including the admonition) for the rest of the page. I'm okay with that tradeoff.

I briefly explored adjusting the style conditionally for pages without sidebars, but I wasn't able to account for presence or absence of the TOC in the selector, so it proved near-impossible to achieve.

Copy link
Contributor

@eak12913 eak12913 left a comment

Choose a reason for hiding this comment

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

It looks pretty darn good - but I wonder if we can get the content centered?

image

I see that you're hiding the the TOC - but I'm wondering if we need more of a centered layout for this page.

Edit: I see that you're explicitly NOT hiding the TOC but making use of the fact that we've modified the component. I wonder if in this case - we want something else. Perhaps this is where we use our own Layout?

@ebeneliason
Copy link
Contributor Author

Womp.

Custom layout got removed because upon investigation it actually doesn't work 😭

Let's leave custom layout on v2 implementation

Custom layouts don't appear to be available to us just yet. I would love to center it, but found no way to do so that wouldn't interfere with other pages, or result in an extra wide layout (when hiding the TOC).

@ebeneliason
Copy link
Contributor Author

We found a solution for centering by adding an MDX wrapper to the content.

Copy link
Contributor

@eak12913 eak12913 left a comment

Choose a reason for hiding this comment

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

LGTM!

@eak12913 eak12913 merged commit fcfecd3 into master Dec 15, 2021
@eak12913 eak12913 deleted the apt-1585-courses branch December 15, 2021 18:07
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.

2 participants