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 SCSS structure #619

Merged
merged 3 commits into from
Apr 20, 2022
Merged

Refactor SCSS structure #619

merged 3 commits into from
Apr 20, 2022

Conversation

choldgraf
Copy link
Collaborator

@choldgraf choldgraf commented Apr 10, 2022

This refactors our SCSS so that it follows a more standard structure that aligns with sphinx-basic-ng, sphinx-book-theme, furo, etc. I don't believe it changes the end-result of anything, it only updates the location of SCSS files so that they are more modular, extendable, etc.

ref: #577

@choldgraf
Copy link
Collaborator Author

pinging @damianavila on this one - this will break quickly when people make PRs, so if possible it would be great to get this one merged soon. I'm happy to iterate on some feedback though!

Copy link
Collaborator

@12rambau 12rambau left a comment

Choose a reason for hiding this comment

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

@choldgraf I really like how you split everything as I love small files with meaningful names, that's way easier o search in an IDE.

I don't know much about the inspiration you're quoting in #577 but I assume you respected some kind of convention and I think it works very well (special thanks for sidebar-primary and sidebar-secondary).

I have 2 questions though:

  • why the underscores? I use "_" to differentiate private from public stuff in Python but here everything is the same so why bother?
  • any reason why you created empty files (_toc-inpage, _tables, _variables)?

I don't feel legit to give any formal approval but I like the new structure!

@damianavila
Copy link
Collaborator

@choldgraf, some comments:

  1. This looks great to me (I left some tiny comments). I love the new structure!!
  2. This PR already has some merge conflicts because of some recent merges. Let's stop merging new stuff until we land this one.
  3. Let's merge this one in the next 24-48 hs.

@choldgraf
Copy link
Collaborator Author

OK I believe that I've cleaned up this PR and it's ready to merge - conflicts are gone and I've added back in the missing files that you mentioned, thanks for catching that @12rambau !

Copy link
Collaborator

@damianavila damianavila left a comment

Choose a reason for hiding this comment

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

LGTM and merging right away so it does not get old! 😉

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.

4 participants