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

IBX-1802: Update design for location swap and table locations #146

Merged
merged 4 commits into from
Dec 30, 2021

Conversation

lucasOsti
Copy link
Contributor

@lucasOsti lucasOsti commented Dec 28, 2021

Question Answer
Tickets https://issues.ibexa.co/browse/IBX-1802
Bug fix? yes
New feature? no
BC breaks? no
Tests pass? yes
Doc needed? no
License GPL-2.0

Screenshot 2021-12-28 at 14 07 37

Checklist:

  • Coding standards ($ composer fix-cs)
  • Ready for Code Review

@@ -0,0 +1,12 @@
.ibexa-form-location-swap {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.ibexa-form-location-swap {
.ibexa-location-swap-form {

name="updateVisibility"
{{ not can_hide[location.id] ? 'disabled' }}
class="ibexa-input ibexa-input--radio"
type="radio"
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure? I think that the old type (checkbox) is the correct one.

Suggested change
type="radio"
type="checkbox"

Copy link
Contributor Author

@lucasOsti lucasOsti Dec 30, 2021

Choose a reason for hiding this comment

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

According to the invsion project, there should be a radio here
Current:
Screenshot 2021-12-30 at 09 23 08
Invsion:
https://projects.invisionapp.com/d/main#/console/21797486/461599772/preview

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, you are right, I was confused because the order of the columns has been changed. :)


{% set col_raw %}
<label
class="ibexa-checkbox-icon {{ not location.explicitlyHidden ? 'is-checked' }}{{ not can_hide[location.id] ? 'disabled' }}"
Copy link
Contributor

@tischsoic tischsoic Dec 29, 2021

Choose a reason for hiding this comment

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

Can we delete ibexa-checkbox-icon styles? I may be wrong, but it seems that it is only used here.
(There is one occurrence in JS in Page Builder, but it is only JS and probably is a dead code and should be deleted.)

{% endembed %}
{% set col_raw %}
<div class="ibexa-content-locations__visibility-toggler">
<div class="ibexa-toggle ibexa-toggle--checkbox {{ not can_hide[location.id] ? 'ibexa-toggle--is-disabled' }} {{ not location.explicitlyHidden ? 'ibexa-toggle--is-checked' }}">
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we import toggle_widget.html.twig here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess I can't use the toggle widget here because it is a block

Copy link
Contributor

Choose a reason for hiding this comment

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

As discussed in private: for now, it is not possible to include toggle_widget.html.twig.

@dew326 dew326 merged commit 9601720 into ibexa:main Dec 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants