Skip to content

Comments

Ability to order versions list descending#304

Merged
filipac merged 7 commits intorainlab:masterfrom
filipac:sort-versions
Nov 4, 2019
Merged

Ability to order versions list descending#304
filipac merged 7 commits intorainlab:masterfrom
filipac:sort-versions

Conversation

@filipac
Copy link
Contributor

@filipac filipac commented Oct 17, 2019

While working on a big private plugin, I quite often go into the versions list of builder to create new manually migrations. After I edit those in an external editor, I have to re-open it in Builder to get new code before applying. When you have hundreds of versions, it's quite time consuming to scroll to the bottom every time you refresh. Example:
Oct-17-2019 15-33-09

This could be easily fixed by adding a new button on version list to reverse the order. This is my PR esentially:

  • Order versions ascending or descening
  • Order is saved in session

After my changes:
Oct-17-2019 15-35-34

Changes are welcomed before merging.

@bennothommo
Copy link
Contributor

@filipac It's a good idea, however, I don't think the button to sort the versions should be in the orange section, as that's reserved for the plugin name. The conventional placement for that button would be next to the "Add" button, like it is for the "CMS" section of the Backend.

image

Would you mind moving it into there and styling it appropriately? Once done, feel free to send us a screenshot.

@@ -0,0 +1,8 @@
<div class="pull-right">
<?php if($this->getConfig('sort', 'asc') === 'asc'): ?>
Copy link
Contributor

Choose a reason for hiding this comment

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

@filipac Please add a space between the if and (.

Suggested change
<?php if($this->getConfig('sort', 'asc') === 'asc'): ?>
<?php if ($this->getConfig('sort', 'asc') === 'asc'): ?>

@@ -0,0 +1,8 @@
<div class="pull-right">
<?php if($this->getConfig('sort', 'asc') === 'asc'): ?>
<a href="javascript:;" data-request="<?= $this->getEventHandler('onSort') ?>" data-request-data="sort: 'desc'" class="white oc-icon-sort-numeric-desc" title="<?= e(trans('rainlab.builder::lang.version.sort_descending')) ?>"></a>
Copy link
Contributor

Choose a reason for hiding this comment

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

@filipac Please indent this 4 spaces to the right to nest it inside the condition. Please also do the same for the line below the <?php else: ?> line.

$items = $result;
}

if($this->getConfig('sort', 'asc') == 'desc') {
Copy link
Contributor

Choose a reason for hiding this comment

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

@filipac Space between if and (.

Suggested change
if($this->getConfig('sort', 'asc') == 'desc') {
if ($this->getConfig('sort', 'asc') == 'desc') {


public function onSort()
{
$this->config->sort = request()->sort;
Copy link
Contributor

Choose a reason for hiding this comment

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

@filipac We prefer using the facade for reading data.

Suggested change
$this->config->sort = request()->sort;
$this->config->sort = Request::input('sort');

@filipac
Copy link
Contributor Author

filipac commented Oct 17, 2019

Made the change + code style you suggested.
image

@filipac filipac requested a review from bennothommo October 17, 2019 13:23
filipac and others added 3 commits October 17, 2019 18:08
Co-Authored-By: Luke Towers <github@luketowers.ca>
Co-Authored-By: Luke Towers <github@luketowers.ca>
@filipac
Copy link
Contributor Author

filipac commented Oct 17, 2019

done.

@filipac filipac requested a review from LukeTowers October 17, 2019 20:47
@filipac
Copy link
Contributor Author

filipac commented Oct 21, 2019

@LukeTowers @bennothommo any updates on this required? :D i did composer update and lost my local changes and i miss the order :))

@w20k
Copy link

w20k commented Oct 22, 2019

@filipac CSS is missing still😄 Buttons are messed up.

Screenshot (will add a screenshot, when GitHub will let me. Uploaded on free hosting for now):
https://i.ibb.co/Hqn2Fh6/Screenshot-2019-10-22-at-16-18-10.png

@bennothommo
Copy link
Contributor

@filipac If you are still interested in getting this merged, please review @w20k's comment above.

@filipac filipac merged commit fb681fe into rainlab:master Nov 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants