-
Notifications
You must be signed in to change notification settings - Fork 82
✨ Student Review Component #2285
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
base: 4.0.0-dev
Are you sure you want to change the base?
Conversation
…rmalinks, and refactor the given reviews template.
…dd review action transition, and remove debug output.
…/delete functionality, new API endpoints, and associated UI components.
…tandardize review form field names.
…omponent and update associated frontend logic
… successful review deletion.
|
|
||
| export const initializeReviews = () => { | ||
| window.TutorComponentRegistry.registerAll({ | ||
| components: [reviewCardMeta, reviewServicesMeta], |
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.
Both will be initialized at the same time. I think it's best to keep everything inside one function. There are only 2 mutations.
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.
Both will be initialized at the same time, but they are not used in the same level of DOM structure. reviewServicesMeta is used at an upper level to update deleteModal without creating a new one for each review-card.
| transition-property: background-color, border-color, opacity; | ||
| transition-duration: 0.2s; | ||
| transition-timing-function: ease-in-out; | ||
| @include tutor-transition((background-color, border-color, opacity)); |
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.
Why double bracket?
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.
This is the first parameter of the mixing, without brackets, it will be considered as three different parameters.
| return $this; | ||
| } | ||
|
|
||
| /** |
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.
We can determine if icon only or not programmatically. No need to add prop support.
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.
Prop support is needed, as label will be used as aria-label for accessibility.
| $content = self::POSITION_RIGHT === ( $this->icon_position ? $this->icon_position : self::POSITION_LEFT ) | ||
| ? sprintf( '%1$s%2$s', esc_html( $this->label ), $icon_html ) | ||
| : sprintf( '%1$s%2$s', $icon_html, esc_html( $this->label ) ); | ||
| if ( $this->icon_only ) { |
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.
This is not necessary I guess.
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.
When icon-only, we don't need to perform those checks; it's better to leave early when possible.
| if ( ! defined( 'ABSPATH' ) ) { | ||
| exit; | ||
| } | ||
| use TUTOR\Icon; |
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.
Why are we still working on demo components?
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.
Removed demo component and created new StarRating component.
| @submit.prevent="handleSubmit( | ||
| (data) => handleReviewSubmit(data), | ||
| (errors) => { | ||
| console.log('Form errors:', errors); |
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.
Display toast for errors.
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.
This was not intentional. Removing this since we are handling errors inside query.
| <div class="tutor-review-rating"> | ||
| <?php | ||
| tutor_load_template( | ||
| 'demo-components.dashboard.components.star-rating', |
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.
Why demo-components?
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.
Used newly created component.
| ->message( __( 'Are you sure you want to delete this review? Please confirm your choice.', 'tutor' ) ) | ||
| ->confirm_handler( 'handleDeleteReview(payload?.id)' ) | ||
| ->mutation_state( 'deleteReviewMutation' ) | ||
| ->confirm_button( |
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.
No need to provide confirm_button and cancel_button. the default should work.
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.
Updated.
| </ul> | ||
| </div> | ||
| <?php endif; ?> | ||
| $converted_reviews = array_map( |
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.
Try to skip this. Update the review-card template accordingly.
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.
Updated
| return wpAjaxInstance.post(endpoints.PLACE_RATING, payload).then((res) => res.data); | ||
| }, | ||
|
|
||
| convertFormDataToPayload(data: ReviewFormProps): ReviewPayload { |
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.
Try to skip this conversion.
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.
This is needed as the payload does not match the input names.
…e` and `icon_size` for consistency.
…confirm_text` and `cancel_text` for modal actions.
…nd remove redundant data conversion.
…f `id` and `review_content`.
| 'paged' => $current_page, | ||
| ); | ||
|
|
||
| $pagination_template_frontend = tutor()->path . 'templates/dashboard/elements/pagination.php'; |
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 use pagination component.
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.
This is part of the instructor design, which is not here yet. So, I kept the previous code as it was.
| </table> | ||
| </div> | ||
| <?php else : ?> | ||
| <?php tutor_utils()->tutor_empty_state( tutor_utils()->not_found_text() ); ?> |
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 use empty state component
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.
This is part of the instructor design, which is not here yet. So, I kept the previous code as it was.
| <?php endif; ?> | ||
|
|
||
| <?php if ( $reviews->count ) : ?> | ||
| <div class="tutor-table-responsive"> |
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.
Where is this design?
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.
Same here.
| * | ||
| * @return $this | ||
| */ | ||
| public function error( $error ) { |
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.
Why this method removed?
| </span> | ||
| <?php endif; ?> | ||
|
|
||
| <?php if ( null !== $this->count ) : ?> |
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.
We write simply
if ( $this->count )
| <?php endfor; ?> | ||
| </div> | ||
|
|
||
| <?php if ( $this->show_average || null !== $this->count ) : ?> |
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.
if ( $this->show_average || $this->count )| * | ||
| * @var string | ||
| */ | ||
| protected $field_name = ''; |
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.
The default name should be rating
| * | ||
| * @var int | ||
| */ | ||
| protected $icon_size = 20; |
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.
We can use Size::SIZE_20
| * @link https://themeum.com | ||
| * @since 4.0.0 | ||
| */ | ||
|
|
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.
Prevent direct access
| * @since 4.0.0 | ||
| */ | ||
|
|
||
| use TUTOR\Icon; |
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.
Revert this file changes. This is not part of this PR
| @@ -1,87 +0,0 @@ | |||
| <?php | |||
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.
Why this file has been removed?
| 'learning-area' => 'Learning Area', | ||
| 'user-profile' => 'User Profile', | ||
| 'certificates' => 'Certificates', | ||
| 'reviews' => 'Reviews', |
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.
Do not remove
| @@ -1,27 +0,0 @@ | |||
| <?php | |||
| /** | |||
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.
Do not remove demo component now
In this PR:
StarRatingInput,StartRatingcomponentsInputFieldcomponent bugfixButtoncomponent icon-only variant