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

Move colorItems to ItemList #3186

Merged
merged 2 commits into from
Dec 2, 2021
Merged

Move colorItems to ItemList #3186

merged 2 commits into from
Dec 2, 2021

Conversation

imorland
Copy link
Member

@imorland imorland commented Dec 2, 2021

Changes proposed in this pull request:
At present, if an extension or theme wishes to add/remove any items from Colors, dom traversal is required.
To allow for extensions/themes to better integrate with the Appearance Page, each element under Colors is now built within an ItemList.

Screenshot
Visibly, no changes
image

Necessity

  • Has the problem that is being solved here been clearly explained?
  • If applicable, have various options for solving this problem been considered?
  • For core PRs, does this need to be in core, or could it be in an extension?
  • Are we willing to maintain this for years / potentially forever?

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).
  • Core developer confirmed locally this works as intended.
  • Tests have been added, or are not appropriate here.

Required changes:

  • Related documentation PR: (Remove if irrelevant)
  • Related core extension PRs: (Remove if irrelevant)

@davwheat davwheat added this to the 1.2 milestone Dec 2, 2021
@SychO9
Copy link
Member

SychO9 commented Dec 2, 2021

if an extension/theme is adding a new color variable, shouldn't that be in the extension's own settings page 🤔 ?

@imorland
Copy link
Member Author

imorland commented Dec 2, 2021

A couple of potential use cases:

  • fof/nightmode may want to remove the dark mode toggle, as this might be confusing to have that active/available when "auto night mode" is active via that extension

  • A possible future extension could replace the text input for color hex with a color picker

js/src/admin/components/AppearancePage.js Outdated Show resolved Hide resolved
@askvortsov1
Copy link
Sponsor Member

A few thoughts before merging.

I'm fine with this change for now, but I want to see a big revamp of the admin dashboard as part of 2.0. To the point where we might want to revert some of the changes we made in v1.0. Some particular concerns:

  • It's not clear whether extensions should be putting settings on their own pages, or extending other extension pages / core pages.
  • Some extensions clearly need their own pages, others really don't. There's no visual indication in the UI which is which.
  • Some extensions are really headless plugins rather than full-fledged modules, it would be nice to distinguish between the two.

@askvortsov1 askvortsov1 merged commit 6a90938 into master Dec 2, 2021
@askvortsov1 askvortsov1 deleted the im/appearance branch December 2, 2021 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants