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

Use Sass-style comments in Furo #428

Merged
merged 3 commits into from
Jun 29, 2023

Conversation

Eric-Arellano
Copy link
Collaborator

@Eric-Arellano Eric-Arellano commented Jun 29, 2023

CSS didn't let us use // style comments, but SCSS (Sassy CSS) does. These comments are simpler to write and read.

Uses ::before rather than :before

I found out that it's preferred to use ::before for clarity, e.g. footer::before rather than footer:before.

Moves some related rules closer together

Some code was moved closer to its relevant rules. This is prework for an upcoming refactor to use Sass nesting.

No changes were made -- it only moves the code.

@@ -18,25 +18,35 @@ body {
border: 1px solid var(--qiskit-color-admonition-info--background);
}

/* Admonitions are complex because they use a 3px left border with a different color than the rest
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This comment is moved below because I moved the rules for p.topic-title::before and p.topic-title higher in the file. That will unblock the follow up PR.

@@ -1,50 +1,54 @@
// By default, Furo expands the left and right sidebars when the page width increases, but it
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This comment was moved from down below in the file. I realize it makes sense to be document-level since it explains our overall strategy for the page layout.

No changes made, other than where I comment below.

// sidebar from being split way too far from the page content itself.
//
// The easiest way to achieve this is using flexbox, given that Furo already uses it. We keep
// Furo's default values for `width` and `min-width` for the .sidebar-drawer because it's necessary
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

for the .sidebar-drawer is new.

Comment on lines -31 to -34
.qiskit-translations-header-container label,
.qiskit-previous-releases-header-container label {
cursor: pointer;
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved a little lower

* -------------------------------------------------------------------------------------- */
// --------------------------------------------------------------------------------------
// Translations
// --------------------------------------------------------------------------------------
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I reordered code in the below two sections. No changes made, other than to use // comments.

--color-table-header-background: #dde1e6;
}

/* Because we disable drop shadows, restore borders for tables by overriding rules that disabled
* them. */
// Add a border to the top of the table so that tables without headers have a border.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This rule was moved higher up in the file.

node-version = "18.16.0"
node-version = "18.16.1"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

CI failed because it couldn't download Node 18.16.0. 18.16.1 is the newest release so I figured I'd upgrade while at it.

Copy link
Member

@frankharkins frankharkins left a comment

Choose a reason for hiding this comment

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

I found out that it's preferred to use ::before for clarity, e.g. footer::before rather than footer:before.

TIL; more info.

Otherwise if it's just moving code around then LGTM.

@Eric-Arellano Eric-Arellano merged commit 3776a32 into Qiskit:main Jun 29, 2023
4 checks passed
@Eric-Arellano Eric-Arellano deleted the sass-comments branch June 29, 2023 14:14
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