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.

Sign up for GitHub

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

Jump to bottom

Minor admin ui redesign #210

Merged
merged 5 commits into from
Dec 20, 2021
Merged

Minor admin ui redesign #210

merged 5 commits into from
Dec 20, 2021

Conversation

diimpp
Copy link
Member

@diimpp diimpp commented Feb 24, 2021 •
edited
Loading

  1. Replace channel color feature with sylius/sylius standard template
    • Header of invoice show page
      Before
      image
      After
      image
    • Channel column for invoices index page
      Before
      image
      After
      image
    • Order show page, list of invoices
      Before
      image
      After
      image
  2. Move breadcrumb block at invoices admin show page to correct place as for other CRUD templates
    Before
    image
    After
    image
  3. Move seller/buyer blocks on top of the page, same as for CreditMemo show page
    Before
    image
    After
    image
  4. Wrap action buttons with button group for order's invoices list (Download btn colored blue as secondary action blue in the same schema as for credit memo download btn. "Resend" btn left grey the same as for "resend order confirmation"/"resend shipment confirmation" btns)
    Before
    image
    After
    image
    And credit memos for context (Do ignore slightly non-standard view, artifact of the project)
    image

Before
image
After
image

Let me know if you would like to change something.

@lchrusciel @GSadee @CoderMaggie review please :)

@diimpp diimpp force-pushed the feature/admin_ui_minor_redesign branch from e794944 to 06faf60 Compare February 24, 2021 07:35
@diimpp diimpp marked this pull request as ready for review February 24, 2021 08:29
@diimpp diimpp requested a review from a team as a code owner February 24, 2021 08:29
@CoderMaggie
Copy link
Member

@diimpp to me it looks more than okay, although the build is failing 😭

@diimpp
Copy link
Member Author

diimpp commented Sep 27, 2021 •
edited
Loading

@GSadee hey man, if you're taking part of the PR and merge it in another PR, please don't forget to add mention about that -- I'm spending time to rebase this PR only to see, that some changes already in master. (I'm talking about show invoice layout 06faf60 d3c516d)

@diimpp diimpp force-pushed the feature/admin_ui_minor_redesign branch from f950aee to b20a34e Compare September 27, 2021 12:37
@diimpp
Copy link
Member Author

diimpp commented Sep 27, 2021

@CoderMaggie ready for review, though points 2 and 3 already in master. Please start CI for this PR.

GSadee
GSadee reviewed Nov 30, 2021
View reviewed changes
README.md
> Remember to allow community recipes with `composer config extra.symfony.allow-contrib true` or during plugin installation process

1. Apply migrations to your database:
2. Apply migrations to your database:
Copy link
Member

Choose a reason for hiding this comment

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

There is no need to change numeration, because 1 will be auto-incremented in the view

Copy link
Member Author

Choose a reason for hiding this comment

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

This doesn't sit right with me, this is clearly "fallback" feature for when people missing correct numeration and not target correct syntax.

Copy link
Member

Choose a reason for hiding this comment

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

I'm OK with changing numeration, but you should adjust also line 53 of this file

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I've re-numbered UPGRADE as well.

tests/Behat/Page/Admin/Invoice/ShowPage.php
@@ -138,7 +138,9 @@ public function hasTotal(string $total, string $currencyCode): bool

public function getChannel(): string
{
return $this->getElement('invoice_channel_name')->getText();
$items = $this->getDocument()->findAll('css', '.channel > .channel__item');
Copy link
Member

Choose a reason for hiding this comment

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

Change the value of key invoice_channel_name and use it here

Copy link
Member Author

Choose a reason for hiding this comment

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

This PR replaces channel widget with sylius one, which doesn't have invoice_channel_name id or any other ids.

https://github.com/Sylius/Sylius/blob/master/src/Sylius/Bundle/AdminBundle/Resources/views/Common/_channel.html.twig

Though I don't understand which key you're mentioning.

Copy link
Member

Choose a reason for hiding this comment

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

I'm talking about invoice_channel_name key that was used by getElement method before and it is configured below in getDefinedElements method. You should change the selector there and keep using $this->getElement('invoice_channel_name')->getText(); if it possible

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it's not possible anymore to access the name via static path, so I will remove it from elements.

Implementation is pretty much the same as for RefundPlugin.
https://github.com/Sylius/RefundPlugin/blob/b263bf35834dc8f77a70d0d34ab9b5fc2fcc1453/tests/Behat/Page/Admin/CreditMemoDetailsPage.php#L78

@GSadee GSadee closed this Nov 30, 2021
@GSadee GSadee reopened this Nov 30, 2021
@GSadee
Copy link
Member

GSadee commented Nov 30, 2021

@diimpp please rebase with master to fix the conflict, it should also fix problems with failing builds. I'm also sorry for not answering for a very long time 😞

@CoderMaggie
Copy link
Member

Fingers crossed it will get merged soon! 🤞🏼 🤞🏼 🤞🏼

@diimpp diimpp force-pushed the feature/admin_ui_minor_redesign branch from b20a34e to c498b39 Compare December 3, 2021 08:37
@diimpp
Copy link
Member Author

diimpp commented Dec 3, 2021

@GSadee rebase is done.

@diimpp
Copy link
Member Author

diimpp commented Dec 20, 2021

@GSadee @CoderMaggie ping

@diimpp diimpp requested a review from GSadee December 20, 2021 07:37
@Zales0123 Zales0123 merged commit 779c08f into Sylius:master Dec 20, 2021
@Zales0123
Copy link
Member

Thank you, Dmitri! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Zales0123 Zales0123 Zales0123 approved these changes

@GSadee GSadee GSadee approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@diimpp @CoderMaggie @GSadee @Zales0123