Skip to content
This repository has been archived by the owner on May 25, 2023. It is now read-only.

fix: sorting in ListGoals #1245

Merged
merged 2 commits into from
Oct 21, 2021
Merged

Conversation

murilo-goncalves
Copy link
Contributor

What type of PR is this? (check all applicable)

  • ♻️ Refactor
  • ✨ Feature
  • 🐛 Bug Fix
  • 👷 Optimization
  • 📝 Documentation Update
  • 🔖 Release
  • 🚩 Other

Description

This PR fixes sorting in ListGoals, as discussed in the linked issue.

Related Tickets & Documents

Fixes #1244

Mobile & Desktop Screenshots/Recordings

Before:

before

After:

after

Added tests?

  • 👍 yes
  • 🙅 no, because they aren't needed
  • 🙋 no, because I need help

Added to documentation?

  • 📜 readme
  • 📜 contributing.md
  • 📓 docs
  • 📕 storybook
  • 🙅 no documentation needed

[optional] Are there any post-deployment tasks we need to perform?

Nops

[optional] What gif best describes this PR or how it makes you feel?

Any gif showing happiness, I loved starting to contribute to Open Source! ❤️

@github-actions
Copy link
Contributor

PR Compliance Checks

Thank you for your Pull Request! We have run several checks on this pull request in order to make sure it's suitable for merging into this project. The results are listed in the following section.

Conventional Commit PR Title

In order to be considered for merging, the pull request title must match the specification in @commitlint/conventional-commit. You can edit the title in order for this check to pass.
Most often, our PR titles are something like one of these:

  • docs: correct typo in README
  • feat: implement dark mode"
  • fix: correct remove button behavior

Linting Errors

  • subject must not be sentence-case, start-case, pascal-case, upper-case

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Congrats on making your first Pull Request and thanks for taking the time to improve Open Sauced! ❤️! 🎉🍕
Say hello by joining the conversation in our Discord

@mtfoley mtfoley changed the title fix: Sorting in ListGoals fix: sorting in ListGoals Oct 20, 2021
@mtfoley
Copy link
Contributor

mtfoley commented Oct 20, 2021

Hi @murilo-goncalves I’m so glad you decided to contribute! I hope you don’t mind my editing the title of the PR to fit our compliance checks.

You did a great job of including screenshots to show how the sorting worked for stars.!

Meanwhile as I’m looking over your changes I think you pulled too much of the ternary operation out. In my experience you want to handle cases that return -1,1, and 0. Others may be fine with this as is.

@murilo-goncalves
Copy link
Contributor Author

Hi @murilo-goncalves I’m so glad you decided to contribute! I hope you don’t mind my editing the title of the PR to fit our compliance checks.

You did a great job of including screenshots to show how the sorting worked for stars.!

Meanwhile as I’m looking over your changes I think you pulled too much of the ternary operation out. In my experience you want to handle cases that return -1,1, and 0. Others may be fine with this as is.

Thanks for the reply! No problem editing the title :D

I can change that. Just thought it was clearer this way, because returning 0 or 1 causes the same effect, the items not being swapped doesn't it?

@0-vortex
Copy link
Contributor

Doesn't work for me with any of the sorting methods. Mind my open sauced goals being a bit bugged: https://github.com/0-vortex/open-sauced-goals/issues

Other than that the sorting dropdown is also iterating through closed issues, which could be a potential parallel issue/fix. What are your thoughts on testing this more thoroughly?

@0-vortex
Copy link
Contributor

Doesn't work for me with any of the sorting methods. Mind my open sauced goals being a bit bugged: https://github.com/0-vortex/open-sauced-goals/issues

Other than that the sorting dropdown is also iterating through closed issues, which could be a potential parallel issue/fix. What are your thoughts on testing this more thoroughly?

Tested this again and identified the issue to be related to the way we parse goals data, fixing that in another branch I could test your PR and confirm it is actually working as intended! Will do the necessary issue reporting and merge this as soon as possible, thank you again for your contribution! ❤️

@0-vortex 0-vortex merged commit 1ec989d into open-sauced:main Oct 21, 2021
github-actions bot pushed a commit that referenced this pull request Oct 21, 2021
### [0.36.1](v0.36.0...v0.36.1) (2021-10-21)

### Bug Fixes

* correct sorting in ListGoals component ([#1245](#1245)) ([1ec989d](1ec989d)), closes [#1244](#1244)
@github-actions
Copy link
Contributor

🎉 This PR is included in version 0.36.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Sorting (probably) broken in ListGoals
3 participants