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

Split Miscellaneous.less into several files #4075

Merged
merged 16 commits into from
Apr 24, 2024
Merged

Split Miscellaneous.less into several files #4075

merged 16 commits into from
Apr 24, 2024

Conversation

hjpalpha
Copy link
Collaborator

@hjpalpha hjpalpha commented Mar 7, 2024

DO NOT MERGE

to do

  • after review (with approval) but before merge: create the files on commons and update resourceloader

resolves #3765

Summary

Miscellaneous.less currently has 5534 lines
this PR extracts styles from it into sep. files. and reduces the number of lines in Miscellaneous.less to 2782

Please note this only extracts from an existing file into several files, it does not do any actual changes to the styles at all.

How did you test this change?

on darkrais dev wiki

@hjpalpha hjpalpha added chore stylesheets changes to stylesheets labels Mar 7, 2024
@hjpalpha hjpalpha mentioned this pull request Mar 7, 2024
@hjpalpha
Copy link
Collaborator Author

hjpalpha commented Mar 7, 2024

martin:
can it cause issues to "change order" of loading stuff?
like we currently load it together with misc, if we extract, might it load after everything else now and hence break stuff?

@Rathoz @FO-nTTaX

@FO-nTTaX
Copy link
Member

FO-nTTaX commented Mar 7, 2024

It potentially can if two selectors have the same specificity

so it is processed after DivTable.less since it uses them
so it is processed after DIvTables.less
@hjpalpha
Copy link
Collaborator Author

hjpalpha commented Mar 8, 2024

It potentially can if two selectors have the same specificity

did some 2 renames to account for it, should be all fine now
will test on a dev wiki in the upcomming days

@hjpalpha hjpalpha marked this pull request as ready for review March 15, 2024 10:28
@iMarbot
Copy link
Collaborator

iMarbot commented Mar 18, 2024

There should realistically not be any cross-interaction between different sections of the Misc file, so splitting these into their own files will be fine in that regard.

However, ResourceLoaderArticles and commons/load.php loads the files in alphabetical order (as far as I can tell), so it could cause conflicts with existing stuff like Colours.less or Tables.less which are both loaded on different sides of the existing misc stuff.

I'm not a massive fan of just merging and fixing if stuff breaks after the fact as we might not stumble upon issues until months down the line.

One solution could be to name all the files Miscellaneous/<new name>.less so they are loaded in basically the existing order, but also not a fan of that because of it being unnecessary convoluted naming.

Another solution (cc @FO-nTTaX), would be to add another (optional?) key to the database such as rla_priority that is ordered by first before rla_page, so that we can specify the order we want to load the styles in here?

https://github.com/Liquipedia/ResourceLoaderArticles/blob/9ae03a8aa39212b6569f850be14567d2834607ec/src/Hooks/MainHookHandler.php#L46C5-L46C65

I don't mind investing some time into adding priority stuff into the extension if it's worthwhile. Don't have a mediawiki setup to test it on however, so would need testing by someone with a dev wiki.

@FO-nTTaX
Copy link
Member

Adding a priority field to ResourceLoaderArticle could be done, making it optional would be weird though. I'd be more than happy to code review and test any changes there.

@Rathoz
Copy link
Collaborator

Rathoz commented Apr 15, 2024

stylesheets/commons/Miscellaneous.less
1275:1 ✖ Unclosed block CssSyntaxError

image

Removed the closing bracket (by mistake) in the merge conflict resolve I assume

iMarbot

This comment was marked as outdated.

@iMarbot
Copy link
Collaborator

iMarbot commented Apr 16, 2024

Now that RLA has priority support, f30eef0 isn't strictly needed, but do we want to keep the files organized like that still or nah?

@Rathoz
Copy link
Collaborator

Rathoz commented Apr 16, 2024

nah I think, miscellaneous is not a category enough :D

@hjpalpha
Copy link
Collaborator Author

lmk if the PR is okay, so i can do the adjusts on commons (creating the new files & adjusting ressource loader) before merge

stylesheets/commons/BigMatch.less Outdated Show resolved Hide resolved
stylesheets/commons/DeckList.less Outdated Show resolved Hide resolved
@hjpalpha hjpalpha requested a review from iMarbot April 17, 2024 08:05
@Rathoz Rathoz requested a review from liquidely April 17, 2024 15:37
@Rathoz Rathoz merged commit 2badf7f into main Apr 24, 2024
5 checks passed
@Rathoz Rathoz deleted the split-misc-less branch April 24, 2024 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore stylesheets changes to stylesheets
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor stylesheets/commons/Miscellaneous.less
5 participants