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.

Sign up for GitHub

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

Jump to bottom

AdminUX Patch and Admin Page #2593

Merged
merged 15 commits into from Feb 18, 2021
Merged

AdminUX Patch and Admin Page #2593

merged 15 commits into from Feb 18, 2021

Conversation

KyrneDev
Copy link
Contributor

@KyrneDev KyrneDev commented Feb 4, 2021 •
edited

Fixes #2498
Fixes #2503
Fixes #2504
Fixes #2520

Changes proposed in this pull request:

  • Add an AdminPage component that all admin pages extend. It contains the buildSettingsComponent helper and a few other helpful save functions.
  • buildSettingsComponent now assigns attrs to the components it is building (min/max/classname etc). Also allows built in helptext.
  • Add padding to the bottom of extension page for permissions
  • Remove AdminLinkButton (breaking/deprecated), AddExtensionModal, and app.extensionSettings. Drop support for SettingsModals for extensions.
  • Prevent scope changes and allow horizontal scrolling from the extension permissions grid.
  • Split ExtensionWidget functions of JSX.
  • LESS despecifications.

Reviewers should focus on:
AdminPage, specifically the info section. Is this how we want to do it or do you have any better ideas?

Confirmed

@KyrneDev KyrneDev self-assigned this Feb 4, 2021
@KyrneDev KyrneDev mentioned this pull request Feb 4, 2021
Merged
@KyrneDev KyrneDev mentioned this pull request Feb 4, 2021
Closed
dsevillamartin
dsevillamartin reviewed Feb 5, 2021
View reviewed changes
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.

This PR is a bit hard, there's many changes that address different things.
I do have a few things to point out and/or ask about.

I think we want methods for the options in info() instead of returning an object.

Why were fields removed in BasicsPage and MailPage? I don't see a reason given in the PR description.

askvortsov1
askvortsov1 requested changes Feb 5, 2021
View reviewed changes
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.

Overall makes sense, left some code nitpicks. Will test locally later.

Going forward, it would be preferable to split big changes like this up into commits (or even better, separate PRs when possible) to make review easier.

js/src/admin/components/AppearancePage.js Outdated Show resolved Hide resolved
js/src/admin/components/BasicsPage.js Outdated Show resolved Hide resolved
@KyrneDev
Copy link
Contributor Author

KyrneDev commented Feb 7, 2021 •
edited

I think we want methods for the options in info() instead of returning an object.

Like icon() and description()?

Why were fields removed in BasicsPage and MailPage? I don't see a reason given in the PR description.

There's no need for them as there is a settings helper already apart of AdminPage now.

@datitisev

@dsevillamartin
Copy link
Member

Like icon() and description()?

Yes, that's what other components do. This was just a thought by the way, but I feel like it'd be more intuitive than returning an object. If we wanted a simple info() method we'd probably create a separate class that encapsulates it, I think (or maybe I'm not making sense).

SychO9
SychO9 requested changes Feb 12, 2021
View reviewed changes
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.

I like this, it makes the interface so much more customizable!

js/src/admin/components/AdminPage.js Outdated Show resolved Hide resolved
less/admin/ExtensionPage.less Outdated Show resolved Hide resolved
less/admin/MailPage.less Outdated Show resolved Hide resolved
less/admin/ExtensionWidget.less Outdated Show resolved Hide resolved
js/src/admin/components/BasicsPage.js Outdated Show resolved Hide resolved
@KyrneDev
Copy link
Contributor Author

@datitisev I'm mostly in favor of keeping it inside one function and return an object. I think having functions for all four info items is a bit redundant and although there will have to be a certain level of redundancy this takes up the smallest footprint. It's just as easy to extend as well.

Let's keep discussing! For now, I'm going to just change it to headerInfo

askvortsov1
askvortsov1 reviewed Feb 17, 2021
View reviewed changes
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.

Could any of the CSS common across pages be factored out into an AdminPage.less file?

SychO9
SychO9 requested changes Feb 17, 2021
View reviewed changes
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.

@askvortsov1 there are some pages like Basics, Appearence and Email, that have similar bottom padding and container CSS code, but it's also different in Dashboard, ExtensionPage and PermissionsPage, so I don't think we could

less/admin/ExtensionPage.less Outdated Show resolved Hide resolved
less/admin/AppearancePage.less Outdated Show resolved Hide resolved
@KyrneDev KyrneDev requested a review from SychO9 February 17, 2021 18:09
SychO9
SychO9 requested changes Feb 18, 2021
View reviewed changes
less/admin/PermissionsPage.less Outdated Show resolved Hide resolved
@KyrneDev KyrneDev requested a review from SychO9 February 18, 2021 19:26
SychO9
SychO9 approved these changes Feb 18, 2021
View reviewed changes
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!

@KyrneDev KyrneDev merged commit 71ccdc0 into master Feb 18, 2021
@KyrneDev KyrneDev deleted the ck/adminux-patch branch February 18, 2021 23:45
@KyrneDev KyrneDev mentioned this pull request Feb 19, 2021
Merged
KyrneDev added a commit that referenced this pull request Feb 20, 2021
@KyrneDev
AdminUX Patch and Admin Page (#2593)
8879701 
* AdminPage

* More fixes

* Settings Modal Drop

* Translation and docblock

* settingS

* Convert Fieldset to JSX

* info -> headerInfo, className

* Overflow fixes

* MailPage

* Admin Less

* Basics Page

* Changes

* Cleanup

* Permission Page

* Add padding
askvortsov1 pushed a commit that referenced this pull request Feb 21, 2021
@KyrneDev @askvortsov1
AdminUX Patch and Admin Page (#2593)
e40fed5 
* AdminPage

* More fixes

* Settings Modal Drop

* Translation and docblock

* settingS

* Convert Fieldset to JSX

* info -> headerInfo, className

* Overflow fixes

* MailPage

* Admin Less

* Basics Page

* Changes

* Cleanup

* Permission Page

* Add padding
@clarkwinkelmann clarkwinkelmann mentioned this pull request Oct 23, 2021
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@askvortsov1 askvortsov1 askvortsov1 approved these changes

@SychO9 SychO9 SychO9 approved these changes

@dsevillamartin dsevillamartin Awaiting requested review from dsevillamartin

Assignees

@KyrneDev KyrneDev

Labels
type/cleanup
Projects
None yet
Milestone
No milestone
4 participants
@KyrneDev @dsevillamartin @SychO9 @askvortsov1