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

Add additional sorting value to ensure the same order of found posts #1737

Merged
merged 1 commit into from
May 12, 2020
Merged

Add additional sorting value to ensure the same order of found posts #1737

merged 1 commit into from
May 12, 2020

Conversation

miina
Copy link
Contributor

@miina miina commented May 12, 2020 •
edited

Summary

Adds additional sorting value to ensure the same order of found posts. Otherwise, there could be duplicate entries on different pages.

Relevant Technical Choices

To-do

Fixes #1726

@miina miina requested a review from BrittanyIRL May 12, 2020 10:30
@miina
Copy link
Contributor Author

miina commented May 12, 2020 •
edited

@BrittanyIRL Could you test to confirm if it fixes #1726 locally for you?

@codecov
Copy link

codecov bot commented May 12, 2020 •
edited

Codecov Report

Merging #1737 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1737   +/-   ##
=========================================
  Coverage     63.63%   63.63%           
  Complexity      216      216           
=========================================
  Files           573      573           
  Lines          9262     9262           
=========================================
  Hits           5894     5894           
  Misses         3368     3368
Impacted Files Coverage Δ Complexity Δ
includes/REST_API/Stories_Controller.php 38.55% <100.00%> (ø) 28.00 <0.00> (ø)

@github-actions
Copy link
Contributor

Size Change: 0 B

Total Size: 855 kB

ℹ️ View Unchanged
Filename Size Change
assets/css/edit-story.css 3.04 kB 0 B
assets/css/stories-dashboard.css 3.04 kB 0 B
assets/js/edit-story.js 406 kB 0 B
assets/js/stories-dashboard.js 443 kB 0 B

compressed-size-action

spacedmonkey
spacedmonkey approved these changes May 12, 2020
View reviewed changes
Copy link
Contributor

@spacedmonkey spacedmonkey left a comment

Choose a reason for hiding this comment

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

I didn't manage to replicate the issue locally. But this change look valid and tests are passing so I will approve.

@BrittanyIRL
Copy link
Contributor

@BrittanyIRL Could you test to confirm if it fixes #1726 locally for you?

Wow, thank you for getting to this so fast! It works great for descending (alpha a to z) but it looks like there's no option to set it to ascending? If you toggle over to the 'list' view, there's the added option to swap and go z - a. Any chance implementing that here is a quick win? Otherwise it can be its own ticket since i think it's something i overlooked in my initial request for this functionality.

author sort list

@miina
Copy link
Contributor Author

miina commented May 12, 2020

@BrittanyIRL Just to confirm -- in case of ascending, should the current author be the last independent of the name, and the rest of the authors come first?

story_author has the enforced logic of displaying the current author as the first. If the goal is to display authors just by the name (independent of the current author) then just author can be used for orderby.

Thoughts?

@BrittanyIRL
Copy link
Contributor

BrittanyIRL commented May 12, 2020 •
edited

@BrittanyIRL Just to confirm -- in case of ascending, should the current author be the last independent of the name, and the rest of the authors come first?

story_author has the enforced logic of displaying the current author as the first. If the goal is to display authors just by the name (independent of the current author) then just author can be used for orderby.

Thoughts?

@miina -
OH that's a great point. I think we need Sam or Paul's help on this - thoughts on doing that separately? I know you're on a different pod now and I think that your PR solves the bug.

BrittanyIRL
BrittanyIRL approved these changes May 12, 2020
View reviewed changes
Copy link
Contributor

@BrittanyIRL BrittanyIRL left a comment

Choose a reason for hiding this comment

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

Changes work great! Thank you for the quick fix on this.

@miina miina merged commit 710576a into master May 12, 2020
@miina miina deleted the fix/1762-story_author_order branch May 12, 2020 15:57
@BrittanyIRL BrittanyIRL mentioned this pull request May 12, 2020
Closed
obetomuniz added a commit that referenced this pull request May 13, 2020
@obetomuniz
Merge branch 'master' into fix/text-box-not-resizing
5bab938 
* master: (99 commits)
  Bump eslint-plugin-react from 7.19.0 to 7.20.0 (#1749)
  Checked if date is moment before wrapping
  Refactored formatted date tests
  Add spy to standardize timing on tests
  Remove tests that used to work in CI but no longer do
  Remove unused import
  Fix snapshot test for useTemplateAPI
  Added support for modal-routing on dashboard.
  Add additional sorting value to ensure the same order. (#1737)
  update tests around reshaping story object
  [ImgBot] Optimize images (#1723)
  Bump @testing-library/dom from 7.5.1 to 7.5.2 (#1732)
  Fixes element measuring relative to fullbleed (better) (#1558)
  swapping to use the decoded html version of the title we have access to to turn html symbols from &amp; to characters &
  adding check for user name to display author
  moving required for users prop type to component level so that we can reuse the shape without it being required. updating displayDate propType to object not string
  some organization of imports
  moving date formatting to the card title level and memoizing
  clean up translation for modified date string in card item
  no need to call edit function inline - this has no functional difference but still
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spacedmonkey spacedmonkey spacedmonkey approved these changes

@BrittanyIRL BrittanyIRL BrittanyIRL approved these changes

@swissspidy swissspidy Awaiting requested review from swissspidy

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

Successfully merging this pull request may close these issues.

Dashboard not getting returned all stories when sorting by author
4 participants
@miina @BrittanyIRL @spacedmonkey @googlebot