-
Notifications
You must be signed in to change notification settings - Fork 2
Feat: implementing complete banner management #4
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
Conversation
|
@SlimDeluxe This PR is ready for a review. Please take a look when you are free. Thanks |
SlimDeluxe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty good 👍 , but some changes are needed:
- Should say "Banners". The term "Positions" should not be exposed too much to the user, since this is something that developers set up once and a normal admin user never changes.
- Should say "New position", it's clear from the context
- Clicking on a position can open the View page like now, but the crossed sections should not be visible. The main focus here is to manage/upload banner images, which is what most admins do.
See the changed labels (for your example "Website Banners") and additions below. You need to make it possible to fully manage banners in this screen, and not in "Edit position". In the end, "edit position" and common banner management are very different things here and should be separate, and banner management should be the primary focus and easily reachable (with as few clicks as possible).
- In position edit, make this a section that wraps the image types records below.
-
Also remove banner management from here.
-
In banner creation, if HIDPI is set, do not require 2 uploads. The user should upload only the 2x size. The label could be improved, e.g. "Mobile @2x (1600×800, displayed as 800×400). The 1x size can be generated by the app — please see if you can do it on upload.
-
Banners should be sortable in the table.
-
See comment below
…d resource with HiDPI support
|
@SlimDeluxe Could you review this. Thanks |
SlimDeluxe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You were very close before, but this now went in the wrong direction. It's nothing like I wanted and was in the wireframe.
- The first dimension/point of entry is the list of positions.
- Clicking on each should open a list of banners for that position. The user can create new ones and manage existing ones.
- In that same list the banners can be sorted.
- There isn't even a upload option for the banner images. It only displays the basic record info.
Please go back to the beginning, see the wireframe, my description, the PR comments and if there are some parts that are unclear, please let me know and we can have a call about this.
|
@SlimDeluxe Ya not showing image is a bug. I fix it. And I thought we were focusing on Banner itself so moved to using Banner resource. I think its clear now. By the way I don't think we need view page for the Banner Position at all right? |
|
The problem with your second approach is that you are showing banners from all positions in a single table, but they are completely unrelated. Each of them is a separate "group" that is presented on a "position" (header, footer, sidebar...). We almost never need the View page, so sure, you can delete it. |
|
@SlimDeluxe Now you can review this.
|
SlimDeluxe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much better, but some changes are still required:
- You need to separate the position management (editing) and banner management (upload, delete...). The first version was better in this regard: you had an "Edit" button on the position view page (see number 3 here). But maybe you can keep the position editing here and move the banner management to a new page for BannerPositionResource?
- This "position edit" action should be available only to the user with the correct permission, therefore you need to separate the permissions for each of those. I suggest adding "Manage banners" to the Position resouce permissions, and removing non-relevant ones (I see you tried to do it but you named the method
getPermissions˙ instead ofgetPermissionPrefixes`). Remember, most admins only manage banners and should not be allowed to edit the banner position, since it will most likely break the site. I've had this exact case happen in production with menus and menu items. - Each of the image types of the position should be a separate column here, so they shouldn't all be in one column like this.
In our demo case, Desktop and Mobile should be two columns after the Name column. Clicking on the image should show it in the "lightbox" modal.
SlimDeluxe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see previous PR comment for required changes.
|
@SlimDeluxe Ready for review |
SlimDeluxe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update needed after ImageColumn changes in common
|
@SlimDeluxe Should be good to review this |
SlimDeluxe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a problem here with this class:
You closed the PR for the previewable ImageColumn for some reason?
DataLinx/eclipsephp-common#3
|
@SlimDeluxe You are right that got closed by mistake. I have kept that to the draft thinking that we will migrate all plugin to F4 before we merge the custom ImageColumn. But it should be fine to use that to test this PR. |
feat/banners