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

Refactor and simplify styles and templates #33

Merged
merged 85 commits into from
Nov 9, 2022

Conversation

rogermparent
Copy link
Contributor

@rogermparent rogermparent commented Jun 23, 2022

This PR has three main goals:

  • Remove non-Tailwind PostCSS plugins and have Tailwind fill their place
  • Refactor our layouts into more standard Gatsby layouts
  • Allow HTML defaults to take care of features they do best, stripping away JS that gets in the way

This could have been broken up into more distinct steps, but ultimately seemed like it would be more work to put in the effort to get one part working only to immediately follow up with removing the part we were adapting to- thus, this monster of a PR.

This PR also makes the homepage look a bit better.

One integrated into the respective repos, this PR's changes should fix quite a few issues:

@rogermparent rogermparent self-assigned this Jun 23, 2022
@rogermparent rogermparent temporarily deployed to gatsby-theme-replace-po-zyi6ao June 28, 2022 21:45 Inactive
@rogermparent rogermparent temporarily deployed to gatsby-theme-replace-po-zyi6ao June 28, 2022 21:53 Inactive
@rogermparent rogermparent temporarily deployed to gatsby-theme-replace-po-zyi6ao June 28, 2022 22:03 Inactive
@rogermparent rogermparent marked this pull request as ready for review June 29, 2022 04:12
@rogermparent rogermparent marked this pull request as draft June 29, 2022 04:12
@rogermparent rogermparent temporarily deployed to gatsby-theme-replace-po-ybipag July 5, 2022 17:26 Inactive
@rogermparent rogermparent temporarily deployed to gatsby-theme-replace-po-ybipag July 5, 2022 17:37 Inactive
@rogermparent rogermparent changed the base branch from deploy-example-on-heroku to main July 5, 2022 17:46
@rogermparent rogermparent temporarily deployed to gatsby-theme-replace-po-ybipag July 5, 2022 20:01 Inactive
@rogermparent rogermparent temporarily deployed to gatsby-theme-replace-po-ybipag July 5, 2022 20:03 Inactive
@rogermparent rogermparent temporarily deployed to gatsby-theme-replace-po-cudk5d October 25, 2022 19:15 Inactive
@rogermparent rogermparent temporarily deployed to gatsby-theme-replace-po-cudk5d October 28, 2022 18:33 Inactive
@rogermparent
Copy link
Contributor Author

Thanks again for the review!

Scrollbars are fixed, just needed a CSS tweak on code blocks that must have gotten clobbered.
image

Fixed up the args linker, it was as simple as pointing it to /doc instead of /docs. The defaults should just work with all our sites, but I also made that option configurable in case another site has different requirements.
image

@rogermparent rogermparent temporarily deployed to gatsby-theme-replace-po-cudk5d November 3, 2022 02:55 Inactive
Copy link
Contributor

@yathomasi yathomasi left a comment

Choose a reason for hiding this comment

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

Great work so far. I think we are almost ready to merge this. There seems to be a small padding issue on the mobile view:

  1. https://gatsby-theme-replace-po-cudk5d.herokuapp.com/doc/start
    (seems to have larger padding shrinking the content)
  2. https://dvc.org/doc/start

Screenshot 2022-11-07 at 15 58 45 Screenshot 2022-11-07 at 15 59 01

I think, we can either have the padding removed on mobile view or use max-width method similar to dvc.org right now.

@rogermparent rogermparent mentioned this pull request Nov 9, 2022
@rogermparent rogermparent temporarily deployed to gatsby-theme-replace-po-cudk5d November 9, 2022 05:58 Inactive
@rogermparent
Copy link
Contributor Author

rogermparent commented Nov 9, 2022

I just pushed up a fix to the padding, I had replaced a media query that removed horizontal padding on the content in mobile view without making the tailwind rules that replace it do the same, and the change was subtle enough that I didn't pick it up looking back at it.

image

Thanks again for the great reviews @yathomasi! Sorry there's been so much back-and-forth, I definitely got lost in this PR after a while. I fixed the padding by simply removing px on content on screens below xs, which is currently our mobile breakpoint.

Taking a tangent on xs as a mobile breakpoint: the website is a bit squished looking right at the breakpoing, maybe we could consider using sm in the future and dropping xs considering it's not even a default tailwind screen size.

image

@rogermparent rogermparent temporarily deployed to gatsby-theme-replace-po-cudk5d November 9, 2022 06:06 Inactive
@yathomasi yathomasi temporarily deployed to gatsby-theme-replace-po-cudk5d November 9, 2022 11:21 Inactive
@yathomasi
Copy link
Contributor

With the recent change to allow docsPrefix, the value provided through slug props had changed. It caused the dysfunction of the next and prev buttons and also the Page Title. I have updated to use pathname instead of slug.
screenshot-2022 11 09-17_01_38

Copy link
Contributor

@yathomasi yathomasi left a comment

Choose a reason for hiding this comment

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

I checked thoroughly and everything looks good to me. Finally, we should go ahead and merge this.

Also, I think we should bump a minor version with all these changes.

Comment on lines +6 to +12
xxsMax: { max: '376px' },
xsMax: { max: '572px' },
smMax: { max: '768px' },
mdMax: { max: '1004px' },
xs: '572px',
lg: '1005px',
xl: '1200px'
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I mentioned it earlier too that it would be easier/better if we could use the default values for screen sizes that tailwind uses. But, it's not a blocker as we can continue using the existing tailwind config on consumer websites.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup! we can follow up after. I decided to port the defaults early on in the process in case anything precise would be broken otherwise.

@rogermparent
Copy link
Contributor Author

Bumping to v0.2.0 and merging! Thanks for doing the quick edit to speed this along.

@rogermparent rogermparent temporarily deployed to gatsby-theme-replace-po-cudk5d November 9, 2022 16:53 Inactive
@rogermparent rogermparent merged commit 2bb4003 into main Nov 9, 2022
@rogermparent rogermparent deleted the replace-postcss-with-tailwind branch November 9, 2022 16:56
@rogermparent rogermparent mentioned this pull request Nov 9, 2022
rogermparent added a commit that referenced this pull request Nov 10, 2022
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