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

Add priority field to loaded resources for fine-grained load ordering #9

Merged
merged 12 commits into from
Apr 16, 2024
Merged

Conversation

iMarbot
Copy link
Contributor

@iMarbot iMarbot commented Apr 13, 2024

Summary

As discussed previously in the Lua & Commons Resources repo, adding priority field to RLA extension so that we can order them based on more than just alphabetical order.

Also disabled editing the form fields on the delete form.

How did you test this change?

Untested as I don't have a test wiki. Please test before merging.

Should just be there for information purposes. Can also be misleading if user thinks `rla_page` is used to delete and not the hidden `rla_id`.
Co-authored-by: Alex Winkler <FO-nTTaX@users.noreply.github.com>
@iMarbot iMarbot changed the title Add priority field to loaded resources for fine-grinded load ordering Add priority field to loaded resources for fine-grained load ordering Apr 13, 2024
Copy link

@hjpalpha hjpalpha left a comment

Choose a reason for hiding this comment

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

just something to be aware of:
not sure if desc sort order is the most intuitive
i think arguments can be made for both sort orders
hence imo adding some information on the special page about how the prio works would be a good idea

Explain the load order - higher loaded first
@iMarbot iMarbot requested a review from hjpalpha April 13, 2024 15:14
Copy link

@hjpalpha hjpalpha left a comment

Choose a reason for hiding this comment

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

apart from the sorting question lgtm
mind i am on phone

src/SpecialPage/SpecialResourceLoaderArticles.php Outdated Show resolved Hide resolved
Also added separating header rows in the special page between scripts and styles.
@iMarbot iMarbot requested a review from hjpalpha April 13, 2024 15:49
Copy link

@hjpalpha hjpalpha left a comment

Choose a reason for hiding this comment

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

lgtm on phone

src/SpecialPage/SpecialResourceLoaderArticles.php Outdated Show resolved Hide resolved
src/SpecialPage/SpecialResourceLoaderArticles.php Outdated Show resolved Hide resolved
src/SpecialPage/SpecialResourceLoaderArticles.php Outdated Show resolved Hide resolved
Co-authored-by: Alex Winkler <FO-nTTaX@users.noreply.github.com>
@iMarbot
Copy link
Contributor Author

iMarbot commented Apr 15, 2024

Idk how I didn't see help-message in HTMLForm 🤦

@FO-nTTaX
Copy link
Member

Idk how I didn't see help-message in HTMLForm 🤦

That's why we have code review :P

@iMarbot iMarbot requested a review from FO-nTTaX April 16, 2024 08:20
@FO-nTTaX FO-nTTaX merged commit 1e16d0a into Liquipedia:master Apr 16, 2024
2 checks passed
This pull request was closed.
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.

3 participants