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

AdminUX Overhaul #2409

Merged
merged 61 commits into from
Nov 24, 2020
Merged

AdminUX Overhaul #2409

merged 61 commits into from
Nov 24, 2020

Conversation

KyrneDev
Copy link
Contributor

@KyrneDev KyrneDev commented Oct 24, 2020

Closes #746

Changes proposed in this pull request:
Completely redesign the admin dashboard and extension pages.

Extensions will be given a default page that looks similar to all other extension pages, it can be easily overridden by changing the component in the route. The main benefit of this is that currently extensions either have a modal or have a page on the side and it can sometimes be hard to know where to go to find the settings of each extension. This puts them all in a predictable and consistent location. It also helps by showing extension-specific permissions on their own page, removing the need to sift through the often very large permissions grid to find certain permissions.

It also allows extensions to be categorized (potentially adding custom categories option). It also has a backward compatibility layer for extensions that are using the settings modal at the moment.

Reviewers should focus on:
Please check code quality (specifically the addExtensionPermissions class). There's bound to be something I missed.
Also, I still need to make a PR to flarum/english for the new translation keys.

Screenshot
unknown
unknown-2

Confirmed

  • Frontend changes: tested on a local Flarum installation.

Required changes:

  • Related core extension PRs: (English PR coming soon).

@dsevillamartin dsevillamartin marked this pull request as draft October 24, 2020 21:14
@luceos luceos added this to the 0.1.0-beta.15 milestone Oct 26, 2020
Copy link
Sponsor Member

@askvortsov1 askvortsov1 left a comment

Choose a reason for hiding this comment

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

Another suggestion brought up by David that I really agree with is that it would be nice to keep a page with all extensions (simple tiles?) as an "overview" page. Perhaps we could use David's admin dash design, or revert to the pre-beta7 design for simplicity.

js/src/admin/AdminApplication.js Outdated Show resolved Hide resolved
js/src/admin/components/AppearancePage.js Outdated Show resolved Hide resolved
js/src/admin/components/ExtensionPage.js Outdated Show resolved Hide resolved
js/src/admin/components/ExtensionPage.js Outdated Show resolved Hide resolved
js/src/admin/components/ExtensionPage.js Outdated Show resolved Hide resolved
js/src/admin/components/ExtensionPage.js Outdated Show resolved Hide resolved
js/src/admin/utils/addExtensionPermission.js Outdated Show resolved Hide resolved
js/src/admin/utils/addExtensionPermission.js Outdated Show resolved Hide resolved
js/src/common/components/SelectDropdown.js Outdated Show resolved Hide resolved
js/src/admin/components/AdminNav.js Outdated Show resolved Hide resolved
@KyrneDev
Copy link
Contributor Author

@askvortsov1 and I had a call yesterday to discuss this briefly and came up with 3 main options on how to do the extension pages, specifically the routes..

Option 1: Each extension is registered to its own route - easy to override and change - but it is a lot of routes, feels clunky.

Option 2: Route resolver routes by default for extensions that don’t have/need settings
Extensions that need to add basic settings/permission can use a util to register this data, or if they want a entirely custom page they can register a route. Use route resolver, check in route resolver if page is registered -> skip, don’t handle.

Option 3: Route resolver for all exts, ext can register entirely new component (new/extend), or provide array of components (JSX).

We were leaning towards #2 but wanted to discuss it with everyone. For the util I'm working on a concept, it's looking like a frontend extender chain with a construct argument that takes the ext id (flarum-flags).

@luceos
Copy link
Member

luceos commented Oct 27, 2020

If in #2 you can still fairly easily add content below the default content (permissions and util-generated simple settings), I'd be fine with it. But I do want to see a concept of such code from the perspective of a dev so that we can review whether this is acceptable enough..

@askvortsov1
Copy link
Sponsor Member

As per a recent dev meeting, current thinking is to get this included as early as possible, so we can dogfood it on our local installs during the release. We'd also like to have some permissions overview page, although that might be removed if we find it to be redundant with the sidebar list.

@KyrneDev
Copy link
Contributor Author

KyrneDev commented Oct 29, 2020

@luceos This is what I currently have for registering permissions (globally & for the page) + simple settings

(flarum/flags)

new ExtensionData('flarum-flags')
    // Settings
    .registerData('settings', {
        'flarum-flags.guidelines_url': {
            type: 'text', // This will be inputted into the input tag for the setting (text/number/etc)
            label: app.translator.trans('flarum-flags.admin.settings.guidelines_url_label')
        },
        'flarum-flags.can_flag_own': {
            type: 'boolean', // This can be bool/boolean/checkbox/switch for a switch to show up
            label: app.translator.trans('flarum-flags.admin.settings.flag_own_posts_label')
        }
    })
    // Permissions
    .registerData('permissions', {
        icon: 'fas fa-flag',
        label: app.translator.trans('flarum-flags.admin.permissions.view_flags_label'),
        permission: 'discussion.viewFlags'
    }, 'moderate', 65)

    .registerData('permissions', {
        icon: 'fas fa-flag',
        label: app.translator.trans('flarum-flags.admin.permissions.flag_posts_label'),
        permission: 'discussion.flagPosts'
    }, 'reply', 65);

@askvortsov1
Copy link
Sponsor Member

Would these maintain ordering of permissions and settings?

@KyrneDev
Copy link
Contributor Author

Would these maintain ordering of permissions and settings?

The settings are in the order of the object. The permissions are an item list and have an optional priority - just like current behavior.

@KyrneDev
Copy link
Contributor Author

KyrneDev commented Nov 8, 2020

@askvortsov1 / @luceos It's mostly ready for test installs. It has all backward compatibility layers built-in. First extension has also been converted: flarum/flags#33. Translations: flarum/lang-english#176

@KyrneDev KyrneDev marked this pull request as ready for review November 8, 2020 01:07
Copy link
Member

@SychO9 SychO9 left a comment

Choose a reason for hiding this comment

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

Amazing work! I love it!

When it comes to web design, my spider senses start tingling, so sorry if my comments are a little too much 😆 (just some suggestions)

less/admin/ExtensionWidget.less Outdated Show resolved Hide resolved
less/admin/ExtensionWidget.less Outdated Show resolved Hide resolved
less/admin/ExtensionPage.less Outdated Show resolved Hide resolved
less/admin/AdminNav.less Outdated Show resolved Hide resolved
less/admin/AdminNav.less Outdated Show resolved Hide resolved
less/admin/AdminNav.less Show resolved Hide resolved
less/admin/AdminNav.less Outdated Show resolved Hide resolved
@jaspervriends
Copy link
Contributor

First of all, the updated design admin dashboard design looks great!

Based on your screenshots I'm wondering, would it be possible to have multiple setting pages for a single extension? On the screenshot above, I see you've added a Permissions section, wouldn't it be an idea there's some submenu availability for extensions? The submenu would only be visible when you hover the extension or just when the extension settings page is active, just like WordPress does for their admin dashboard.

I think it would be better than having one page with all settings sections on it.

Hovering a setting page in WordPress:
image

When the settings page is opened:
image

For example the SEO extension. There are 5 pages available from an dropdown currently on the extension page itself, Health check, SEO settings, Sitemap information, Search engine information and Set up SSL, I think it would be nice to have a small submenu available which are in sight directly for the end user.

I'm worried that if there are too many different sections for an extension the admin dashboard will get messy. What are your thoughts about this?

@KyrneDev
Copy link
Contributor Author

KyrneDev commented Nov 17, 2020

@jaspervriends the nav buttons extensions use will now have their own component class that you can extend just like anything else. (https://github.com/KyrneDev/core/blob/master/js/src/admin/components/ExtensionLinkButton.js) It would be pretty easy to add this to your extension.

I personally believe this is a niche case and would be best added via an extension that needs it. Most extensions don't have a massive amount of settings right now.

I obviously don't have the final say in this matter so if a core dev wants to override this feel free and I'll gladly work on it.

@KyrneDev KyrneDev requested a review from SychO9 November 19, 2020 05:59
Copy link
Member

@SychO9 SychO9 left a comment

Choose a reason for hiding this comment

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

Looks good to me!

This is a big one though, we'd need more reviewing.

Copy link
Sponsor Member

@askvortsov1 askvortsov1 left a comment

Choose a reason for hiding this comment

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

Looked over this again, and I'm very happy with it! Outside of any code style suggestions @clarkwinkelmann and @datitisev may have, I would still like to talk a bit about https://github.com/flarum/core/pull/2409/files#r523281272 (and similar passing of an array of settings in registerSettings. Is there a benefit to passing these in as arrays over just passing them individually?

Copy link
Member

@dsevillamartin dsevillamartin left a comment

Choose a reason for hiding this comment

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

I've added some comments. Not all are important, some are just some observations or requests to change new .component() code into JSX.

I do want to make sure of one thing: are extensions able to completely customize their page if they want to? As in, below the "header" content or whatever, have whatever layout and/or components they want? I've not tested this locally or looked at how extensions might do this.
I assume they are, but I just want to make sure.

js/src/admin/compat.js Outdated Show resolved Hide resolved
js/src/admin/components/AdminNav.js Outdated Show resolved Hide resolved
js/src/admin/components/AdminNav.js Outdated Show resolved Hide resolved
js/src/admin/components/AdminNav.js Outdated Show resolved Hide resolved
js/src/admin/components/ExtensionPage.js Outdated Show resolved Hide resolved
js/src/admin/components/ExtensionPage.js Outdated Show resolved Hide resolved
js/src/admin/components/ExtensionPage.js Show resolved Hide resolved
js/src/admin/components/ExtensionPermissionGrid.js Outdated Show resolved Hide resolved
js/src/admin/components/HeaderSecondary.js Outdated Show resolved Hide resolved
js/src/admin/utils/ExtensionData.js Show resolved Hide resolved
js/src/admin/utils/ExtensionData.js Outdated Show resolved Hide resolved
js/src/admin/components/ExtensionPage.js Outdated Show resolved Hide resolved
Copy link
Contributor

@tankerkiller125 tankerkiller125 left a comment

Choose a reason for hiding this comment

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

I've ensured that this PR works locally, everything seems to work well and I was unable to break anything in my testing.

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.

Revamp admin interface
7 participants