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

PositionColummn UI/UX modifications #11147

Merged
merged 7 commits into from Nov 9, 2018

Conversation

Projects
None yet
8 participants
@jolelievre
Contributor

jolelievre commented Oct 24, 2018

Questions Answers
Branch? 1.7.5.x
Description? Feedback about ordering UI impose to change the behavior of PositionColumn, most notably the handle is not on the position column but on the left of the row, forcing us to add an extra column in those cases
Type? improvement
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? See #9746 and PrestaShop/ps_linklist#48
How to test?

This change is Reviewable

@matks matks added the migration label Oct 29, 2018

@jolelievre

This comment has been minimized.

Contributor

jolelievre commented Nov 7, 2018

thanks @mickaelandrieu
of course @sarjon and @matks feedbacks are more than welcome!

@jolelievre

This comment has been minimized.

Contributor

jolelievre commented Nov 7, 2018

@PierreRambaud I made the modifications you suggested, thanks

@sarjon

review done. :)

Show resolved Hide resolved src/Core/Grid/Presenter/GridPresenter.php
}
return
isset($grid['sorting']['order_by']) && 'position' == strtolower($grid['sorting']['order_by']) &&

This comment has been minimized.

@sarjon

sarjon Nov 8, 2018

Member

i dont like that name position is hardcoded. :/ you should be able to use whatever name you want for your PositionColumn dont you think? for example place, spot and etc.

This comment has been minimized.

@jolelievre

jolelievre Nov 8, 2018

Contributor

you're right!! this needs to be fetched from the grid definition, I'm gonna look into it!

This comment has been minimized.

@jolelievre

jolelievre Nov 8, 2018

Contributor

@sarjon I modified the helper so that it is dynamic now
and @PierreRambaud I also renamed the function, is it better for you?

Show resolved Hide resolved src/Core/Exception/DatabaseException.php
Show resolved Hide resolved src/Core/Grid/Presenter/GridPresenter.php

Changes requested

@jolelievre

This comment has been minimized.

Contributor

jolelievre commented Nov 8, 2018

@sarjon @mickaelandrieu @PierreRambaud if everything is ok for you, can someone approve the review, so that this goes to QA?

@PierreRambaud PierreRambaud added this to the 1.7.5.0 milestone Nov 8, 2018

@marionf marionf self-assigned this Nov 9, 2018

@marionf marionf added QA ✔️ and removed waiting for QA labels Nov 9, 2018

@marionf marionf removed their assignment Nov 9, 2018

@matks

This comment has been minimized.

Contributor

matks commented Nov 9, 2018

Thank you @jolelievre

@matks matks merged commit b46c5de into PrestaShop:1.7.5.x Nov 9, 2018

2 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jolelievre jolelievre deleted the jolelievre:position-ux-modif branch Nov 27, 2018

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