-
Notifications
You must be signed in to change notification settings - Fork 1
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
Implement UX #19
Implement UX #19
Conversation
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.
lets review the design with Saar, I think too many things are still open.
public function get_site_parts( array $config ) { | ||
$site_pages = [ | ||
[ | ||
'title' => __( 'Home Page', 'hello-plus' ), | ||
'link' => get_edit_post_link( get_option( 'page_on_front' ) ), | ||
], | ||
]; | ||
|
||
if ( Kits_Library::get_about_page_id() ) { | ||
$site_pages[] = [ | ||
'title' => __( 'About', 'hello-plus' ), | ||
'link' => get_edit_post_link( Kits_Library::get_about_page_id() ), | ||
]; | ||
} | ||
|
||
if ( Kits_Library::get_services_page_id() ) { | ||
$site_pages[] = [ | ||
'title' => __( 'Services', 'hello-plus' ), | ||
'link' => get_edit_post_link( Kits_Library::get_services_page_id() ), | ||
]; | ||
} | ||
|
||
if ( Kits_Library::get_work_page_id() ) { | ||
$site_pages[] = [ | ||
'title' => __( 'Work', 'hello-plus' ), | ||
'link' => get_edit_post_link( Kits_Library::get_work_page_id() ), |
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.
@nicoladj77 the list of pages should be dynamic, per site.
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.
@nuritsha I think we discussed with Saar and he would have liked to get all the pages, but then we decided to do only the kit pages, also for a ux thing, because there could be 10s of pages
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.
@nicoladj77 For now, please use a dynamically created pages list, without assuming specific pages must be there.
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.
@nuritsha done
modules/admin/module.php
Outdated
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.
maybe check in the static is_active
method if wp_admin
is true? and potentially skip the entire module creation
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.
@nuritsha I basically copied the approach used in the plugin top bar. I was unsure whether it made sense or not to fully leverage the plugin top bar, but though it was better this way
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.
@nicoladj77 I think the plugin top-bar is also rendered in the editor, which is not covered by is_admin()
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.
@nuritsha no the top bar in the editor is different, there is a top bar which is shown in the floating elements and in the elementor menu. I added a check to avoid adding the action in the FE
No description provided.