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

Fixes #208

Open
wants to merge 8 commits into
base: dev
Choose a base branch
from
Open

Fixes #208

wants to merge 8 commits into from

Conversation

Matteo-2f5b
Copy link

No description provided.

@AfroThundr3007730 AfroThundr3007730 changed the base branch from master to dev June 1, 2023 03:26
@AfroThundr3007730
Copy link
Member

Rebased onto dev after merging #206

Copy link
Member

@AfroThundr3007730 AfroThundr3007730 left a comment

Choose a reason for hiding this comment

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

Many of these changes will need to be revisited after the merge of #206, which affected some of the same areas as this PR. Any changes that are still relevant would need to be moved to the most appropriate section of the stylesheet, instead of lumped in near the top.

As mentioned in the comments, tread carefully when making changes with a high potential for collateral damage, such as combining !important with low specificity selectors, or changing the behavior of one.

I would also suggest reviewing the contributing guidelines for additional advice.

@@ -504,7 +507,7 @@
.ve-ui-mwSaveDialog-options,
.mw-parser-output #siteContainer div:first-child,
#mw-page-base {
background-color: transparent;
background-color: transparent !important;
Copy link
Member

Choose a reason for hiding this comment

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

This is no longer necessary after the background fix in #206.

Making the table of contents transparent is closer aligned with the original style though.

.tracklist-total-length span,
.tracklist-total-length td {
background-color: var(--gray-3) !important;
}
Copy link
Member

Choose a reason for hiding this comment

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

Instead of adding new blocks for these rules, find one of the aggregated block of selectors implementing this rule and add the selectors there. This style is broken into sections based on the element being modified, and the colors being applied.

.wikitable > * > tr > th,
.wikitable > * > tr > td:not(.break),
.wikitable > * > tr > td:not(.break):not([style*="color"]):not([style*="background"]):not([bgcolor]),
.wikitable th span[style*="background"],
Copy link
Member

Choose a reason for hiding this comment

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

This has a high potential for collateral damage, especially given the amount of custom wikitable colors and styles employed on Wikipedia. Until they have more specific fixes, this rule remains necessary.

I would be hesitant to accept this change without reviewing quite a few wikitables with color scales to ensure it has the desired effect. We man find this starts an endless game of whack-a-mole.

Comment on lines +443 to +448
.infobox-data {
background-color: var(--gray-2) !important;
}
.infobox-header {
border-color: var(--gray-5) !important;
}
Copy link
Member

Choose a reason for hiding this comment

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

These two rules no longer seem to be necessary after #206 was applied. If you find an area where it appears to be needed, we can revisit this one. Also try to avoid low specificity selectors using !important, where possible.

@@ -3721,6 +3762,7 @@
table.wikitable,
button.formBtn {
border: 1px solid var(--gray-5) !important;
background-color: var(--gray-2) !important;
}
Copy link
Member

Choose a reason for hiding this comment

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

This section is handling borders, so adding rules affecting backgrounds is not appropriate here (and will cause collateral damage). A more specific selector should be chosen for this rule as well.

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