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

refresh timepicker component when order type toggled #1

Closed
wants to merge 3 commits into from

Conversation

olieydt
Copy link
Contributor

@olieydt olieydt commented Apr 17, 2020

To fix issue #348

Copy link
Member

@sampoyigi sampoyigi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR. It works but having to render the LocalBox component partial from the CartBox component doesn't sit right with me, maybe we can find a better approach? This would also break when a user uses a different alias name for the LocalBox component.

@olieydt
Copy link
Contributor Author

olieydt commented Apr 19, 2020

I've added a check to make sure the localbox is present. Changing order types will affect the content of the timepicker, so something must be sent out from here. We could also fire an event from the cartbox and have the localbox listen and render the partial. What do you suggest?

@sampoyigi
Copy link
Member

After my comment, I came across where I loaded the cartBox from the checkout component so let's continue with that approach but maybe add an option to specify the component alias when necessary using component properties. Something like, similar to what you've done

'cartBoxAlias' => [
     'label' => 'Specify the CartBox component alias used to refresh the cart after a payment is selected',
     'type' => 'text',
     'default' => 'cartBox',
],
if ($cartBox = $this->controller->findComponentByAlias($this->property('cartBoxAlias'))) {
    $result['#cart-totals'] = $cartBox->renderPartial('@totals');
}

@olieydt
Copy link
Contributor Author

olieydt commented Apr 19, 2020

That sounds good, ive added the property in the above commit.

@sampoyigi
Copy link
Member

Perfect 👍

@sampoyigi
Copy link
Member

Merged

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.

2 participants