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 Small Patch #2468

Merged
merged 28 commits into from
Dec 7, 2020
Merged

AdminUX Overhaul Small Patch #2468

merged 28 commits into from
Dec 7, 2020

Conversation

KyrneDev
Copy link
Contributor

@KyrneDev KyrneDev commented Nov 25, 2020

ref #2409

2 small issues that were missed are patched in this PR.

  1. Extension list items were not hidden on tablets which made it look pretty bad, tablets are setup well to just use the widget.
  2. Use the control color for AdminHeader descriptions to fit the color scheme

@KyrneDev KyrneDev changed the title AdminUX Overhaul Small AdminUX Overhaul Small Patch Nov 25, 2020
less/admin/AdminNav.less Outdated Show resolved Hide resolved
@SychO9
Copy link
Member

SychO9 commented Nov 25, 2020

Oh and I just noticed something else

flarum lan_admin (9)

Looks like we need to give .AdminLinkButton-description whitespace: normal

And give the active item anchor a @bg-color color, instead of applying it to specifically the child items currently

        &.active > a {
          background: @primary-color;
          font-weight: normal;

          .Button-label,
          .Button-icon {
            color: @body-bg;
            font-weight: bold;
          }
        }

Another thing, I noticed this in the code

@media @desktop, @desktop-hd {

We can simplify if by using

@media @desktop-up {

@askvortsov1
Copy link
Sponsor Member

askvortsov1 commented Nov 25, 2020

A few more suggestions:

Putting it all together: https://i.imgur.com/lv8INGu.png

ALSO:

  • If we remove the container div in ExtensionsWidget, and remove that block from the LESS, stuff seems to work without issue
  • If you refresh the page, or navigate to an extension page via the widget links, the AdminNav should scroll so that the currently active element is visible.

@KyrneDev
Copy link
Contributor Author

Ideally we should leave this PR open for a week or so and see if we find anything else.

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
@matteocontrini
Copy link
Contributor

Wouldn't it be perhaps better to have the "No Settings" text (and similar) aligned to the left, instead of being centred? On large screens, at a first glance, it seems a bit "lost" in the UI. Also, when settings and permissions are actually there, they are aligned to the left.

Before:

image

After:

image

Maybe a more verbose message could also help (e.g. "This extension has no settings").

@askvortsov1
Copy link
Sponsor Member

A few more comments:

  • The Permissions page feels a little off: it feels like the group items are a bit too high up in the block, and a bit too much to the right. Also, maybe the grid itself should be indented a tad bit?

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.

This is looking so much better!

less/admin/AdminNav.less Outdated Show resolved Hide resolved
less/admin/ExtensionWidget.less Outdated Show resolved Hide resolved
less/admin/AdminNav.less Show resolved Hide resolved
@KyrneDev
Copy link
Contributor Author

A few more comments:

  • The Permissions page feels a little off: it feels like the group items are a bit too high up in the block, and a bit too much to the right. Also, maybe the grid itself should be indented a tad bit?

Can you show me what you mean by this? I can't see myself.

@askvortsov1
Copy link
Sponsor Member

A few more comments:

  • The Permissions page feels a little off: it feels like the group items are a bit too high up in the block, and a bit too much to the right. Also, maybe the grid itself should be indented a tad bit?

Can you show me what you mean by this? I can't see myself.

It looks better if the PermissionsPage Button Group elements have their top padding set to 10px instead of 8px. Also, for PermissionsPage-groups > container, the side paddings are too large: it looks better with 10px left and right padding. Also, I think I prefer it when the actual PermissionsGrid element has a left padding of 10-15px.

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 from working on the nicknames extension: there should be an option to pass a callback to registerSetting, which has access to the page as the execution context, and could be used to add custom settings inputs or other stuff in between settings. #2484 (comment)

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.

The "No Settings" and "No Permissions" text feels a bit too big. Maybe that could be made a bit smaller and/or shifted a tiny bit to the right?

js/src/admin/components/ExtensionPage.js Outdated Show resolved Hide resolved
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.

It would be great to add a generic AdminPage class to use with ExtensionPage and other core admin pages and other core admin pages. We can then use that to add bottom paddings to the page by default.

Because extension permissions are stuck to the very bottom, and the basics page's save button as well.

less/admin/DashboardPage.less Outdated Show resolved Hide resolved
@askvortsov1
Copy link
Sponsor Member

It would be great to add a generic AdminPage to use with ExtensionPage and other core admin pages. We can then use that to add bottom paddings to the page by default.

That's something I'd like to see too, and furthermore, we could move buildSettingComponent there, and streamline all the admin page code. But I think that's better for a separate PR, since this one is designed to fix and tweak the UX redesign

@SychO9
Copy link
Member

SychO9 commented Dec 4, 2020

That's something I'd like to see too, and furthermore, we could move buildSettingComponent there, and streamline all the admin page code. But I think that's better for a separate PR, since this one is designed to fix and tweak the UX redesign

btw, I only meant a class for now, just so that we can add the bottom padding (we could add the padding differently, but I think this would be the best way)

And yes a component would be awesome, but that's something we can comeback to another time.

@askvortsov1 askvortsov1 merged commit 07a43f5 into flarum:master Dec 7, 2020
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.

4 participants