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

[Shop][Admin] Final UI tweak before alpha release #6440

Merged
merged 17 commits into from
Oct 20, 2016

Conversation

pjedrzejewski
Copy link
Member

Q A
Bug fix? yes
New feature? yes
BC breaks? yes
Related tickets -
License MIT

@pjedrzejewski pjedrzejewski added Admin AdminBundle related issues and PRs. UX Issues and PRs aimed at improving User eXperience. labels Oct 17, 2016
_sylius:
section: admin
template: SyliusAdminBundle:Order:update.html.twig
permission: true
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the permission option still used?

{% else %}
<p><small>{{ 'sylius.ui.no_taxes'|trans }}</small></p>
<p>{{ 'sylius.ui.no_taxes'|trans }}.</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

The . is necessary here?

@@ -20,3 +20,6 @@ sylius_product:
repository: Sylius\Bundle\ProductBundle\Doctrine\ORM\ProductVariantRepository
form:
default: Sylius\Bundle\CoreBundle\Form\Type\Product\ProductVariantType
product_option:
classes:
repository: Sylius\Bundle\ProductBundle\Doctrine\ORM\ProductOptionRepository
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be specified in ProductBundle?

Copy link
Member Author

Choose a reason for hiding this comment

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

Should, but it won't replace the class name inside of Doctrine itself, only for the service if we configure it via xml file in the bundle. This is that old annoying bug, otherwise we cannot use custom methods in grids because they use getRepository.

@@ -64,22 +64,22 @@
* @param SharedStorageInterface $sharedStorage
* @param IndexPageInterface $indexPage
* @param ShowPageInterface $showPage
* @param UpdateShippingAddressPageInterface $updateShippingAddressPage
* @param UpdatePageInterface $updateShippingAddressPage
Copy link
Contributor

Choose a reason for hiding this comment

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

$updateShippingAddressPage -> $updatePage

@@ -4,6 +4,6 @@ sylius_admin_partial_taxon_tree:
defaults:
_controller: sylius.controller.taxon:indexAction
_sylius:
template: SyliusAdminBundle:Taxon:index.html.twig
template: @SyliusAdmin/Taxon/Partial/_tree.html.twig
Copy link
Contributor

Choose a reason for hiding this comment

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

These all values with leading @ should be inside quotation marks - http://symfony.com/blog/new-in-symfony-2-8-yaml-deprecations

@@ -1,7 +1,7 @@
{% set value = 'sylius.ui.' ~ data %}

{% if options.vars.labels is defined %}
{% include [(options.vars.labels ~ ':' ~ data ~ '.html.twig'), '@SyliusUi/Label/_default.html.twig'] with {'value': value} %}
{% include [(options.vars.labels ~ '/' ~ data ~ '.html.twig'), '@SyliusUi/Label/_default.html.twig'] with {'value': value} %}
Copy link
Contributor

Choose a reason for hiding this comment

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

We can use only this line instead of the whole if structure (don't know how it will affect performance though).

<div class="content">
<span class="header">{{ label }}</span>
<div class="description">
{% if not amount == 0 %}-{% endif %}{{ (-1 * amount)|sylius_price(order.currencyCode, order.exchangeRate) }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Here and everywhere else:

{% if not amount == 0 %}-{% endif %}{{ (-1 * amount)|sylius_price(order.currencyCode, order.exchangeRate) }}

can be replaced with just:

{{ amount|sylius_price(order.currencyCode, order.exchangeRate) }}

Seems like it was a workaround for some currently fixed bug.

{{ order.completedAt|format_date }}
</div>
<div class="item">
{% include [('@SyliusAdmin/Order/Label/State' ~ '/' ~ order.state ~ '.html.twig'), '@SyliusUi/Label/_default.html.twig'] with {'value': ('sylius.ui.' ~ order.state)|trans} %}
Copy link
Contributor

Choose a reason for hiding this comment

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

'@SyliusAdmin/Order/Label/State' ~ '/' ~ order.state -> '@SyliusAdmin/Order/Label/State/' ~ order.state

@pjedrzejewski pjedrzejewski changed the title [WIP] Final UI tweak before alpha release [Shop][Admin] Final UI tweak before alpha release Oct 19, 2016
<option name="min">5</option>
<option name="minMessage">sylius.province.code.min_length</option>
<option name="groups">sylius</option>
</constraint>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why removing this? The province shouldn't have less than 5 characters, all should be in the same format e.g. US-FL

Copy link
Member Author

@pjedrzejewski pjedrzejewski Oct 20, 2016

Choose a reason for hiding this comment

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

Cause regexp + length is stupid. Double validation error...

@michalmarcinkowski michalmarcinkowski merged commit ac66be3 into Sylius:master Oct 20, 2016
@michalmarcinkowski
Copy link
Contributor

Great work Paweł! 👍

pamil pushed a commit to pamil/Sylius that referenced this pull request May 7, 2019
[Shop][Admin] Final UI tweak before alpha release
pamil pushed a commit to pamil/Sylius that referenced this pull request May 7, 2019
[Shop][Admin] Final UI tweak before alpha release
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Admin AdminBundle related issues and PRs. UX Issues and PRs aimed at improving User eXperience.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants