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

Migration of order view page messages block #16255

Merged

Conversation

@tomas862
Copy link
Member

tomas862 commented Nov 4, 2019

Questions Answers
Branch? develop
Description? Migrates messages block located in admin-dev/index.php/sell/orders/orders/1/view?
Type? new feature
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? #15828
How to test? Open page admin-dev/index.php/sell/orders/orders/1/view? and try different combinations of entering message then viewing it. There is one change that is different from invision - see below in image . Please rebuild assets.

I used existing component for defining max textarea characters and it rendered this view

Screenshot (30)

instead of the one provided in invision:

Screenshot (31)

if it should be changed like the one in the invision then I should create a new PR updating its design everywhere. What do you think @matks ? (answer from @matks: yes, OK 👍 )


This change is Reviewable

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Nov 4, 2019

I did an early review, everything looks great !

@tomas862 tomas862 force-pushed the tomas862:migration/order-view-page/message-block branch from ced3b13 to 950ee03 Nov 18, 2019
@tomas862 tomas862 changed the title [WIP] Migration of order view page messages block Migration of order view page messages block Nov 18, 2019
@tomas862 tomas862 marked this pull request as ready for review Nov 18, 2019
@tomas862 tomas862 requested a review from PrestaShop/prestashop-core-developers as a code owner Nov 18, 2019

$routesCollection = $this->get('router')->getRouteCollection();

if (

This comment has been minimized.

Copy link
@matks

matks Nov 18, 2019

Contributor

It's a bit hard to understand this if (...) statement, what's going on in there ?

]
),
new Length([
'max' => 600,

This comment has been minimized.

Copy link
@matks

matks Nov 18, 2019

Contributor

Looks like a nice candidate for a domain constant 🤔 or maybe it's only form-related ?

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Nov 18, 2019

@tomas862 review done, a few minor comments 😊

@MatShir

This comment has been minimized.

Copy link

MatShir commented Nov 18, 2019

@tomas862 the label "design review" is for the QA team ^^

@matks matks self-assigned this Nov 20, 2019
@matks

This comment has been minimized.

Copy link
Contributor

matks commented Nov 20, 2019

I fixed a few items, and the other ones can be fixed later. This allows us to move forward with this PR as it's blocking #16262

Items to be done later

@rokaszygmantas

This comment has been minimized.

Copy link
Contributor

rokaszygmantas commented Nov 21, 2019

@matks @sarahdib what is needed to be done for this PR? I see waiting for dev

@matks matks force-pushed the tomas862:migration/order-view-page/message-block branch from 4c684df to f3d78e0 Dec 4, 2019
@matks matks dismissed their stale review via 595b3a7 Dec 4, 2019
@matks matks dismissed their stale review via 9b0e492 Dec 4, 2019
Copy link
Contributor

Progi1984 left a comment

Ever approved by @matks

@Progi1984 Progi1984 merged commit a506260 into PrestaShop:develop Dec 4, 2019
2 checks passed
2 checks passed
PrettyCI Code formatting
Details
Travis CI - Pull Request Build Passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.