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

IBX-455: Added selected content from bookmarks to the top of list content items #1768

Merged
merged 6 commits into from
Jun 17, 2021
Merged

IBX-455: Added selected content from bookmarks to the top of list content items #1768

merged 6 commits into from
Jun 17, 2021

Conversation

mateuszdebinski
Copy link
Contributor

@mateuszdebinski mateuszdebinski commented Jun 1, 2021 •
edited
Loading

Question Answer
Tickets IBX-455
Bug fix? no
New feature? yes
BC breaks? no
Tests pass? yes
Doc needed? no
License GPL-2.0

Checklist:

  • Coding standards ($ composer fix-cs)
  • Ready for Code Review

@mateuszdebinski mateuszdebinski self-assigned this Jun 1, 2021
@mateuszdebinski mateuszdebinski requested a review from a team June 1, 2021 13:46
@mateuszdebinski mateuszdebinski marked this pull request as ready for review June 1, 2021 13:46
ViniTou
ViniTou reviewed Jun 14, 2021
View reviewed changes
Copy link
Contributor

@ViniTou ViniTou left a comment

Choose a reason for hiding this comment

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

I think we should have all that new code wrapped into private method that describes whats going on here. As for now i see search, some shifts and I still dont know why (aside from PR title.).

Steveb-p
Steveb-p requested changes Jun 14, 2021
View reviewed changes
Copy link
Contributor

@Steveb-p Steveb-p left a comment

Choose a reason for hiding this comment

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

I think we should have all that new code wrapped into private method that describes whats going on here. As for now i see search, some shifts and I still dont know why (aside from PR title.).

I second to that. It's confusing enough that it shouldn't be merged in this form.

@alongosz alongosz changed the title IBX-455: Added selected content from bookmarks to the top of list contents IBX-455: Added selected content from bookmarks to the top of list content items Jun 14, 2021
@micszo micszo self-assigned this Jun 14, 2021
@micszo micszo removed their assignment Jun 15, 2021
@bogusez bogusez self-assigned this Jun 15, 2021
@lserwatka lserwatka merged commit 77983bb into 2.2 Jun 17, 2021
@lserwatka lserwatka deleted the IBX-455_selected_item_from_bookmarks_on_first_position branch June 17, 2021 06:39
@lserwatka
Copy link
Member

You can merge it up now

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

@mikadamczyk mikadamczyk mikadamczyk approved these changes

@damianz5 damianz5 damianz5 approved these changes

@alongosz alongosz alongosz approved these changes

@ciastektk ciastektk ciastektk approved these changes

@webhdx webhdx webhdx approved these changes

@ViniTou ViniTou ViniTou approved these changes

@konradoboza konradoboza konradoboza approved these changes

@bogusez bogusez bogusez approved these changes

@barw4 barw4 Awaiting requested review from barw4

@Steveb-p Steveb-p Awaiting requested review from Steveb-p

Assignees

@mateuszdebinski mateuszdebinski

@bogusez bogusez

Labels
Improvement QA approved
Milestone
No milestone