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.

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

Added Toggle sort order command #74

Merged
merged 4 commits into from
Nov 26, 2021
Merged

Added Toggle sort order command #74

merged 4 commits into from
Nov 26, 2021

Conversation

viperet
Copy link
Contributor

@viperet viperet commented Oct 26, 2021

This PR adds the ability to toggle authors' sort order. (#73)

@viperet
Copy link
Contributor Author

viperet commented Oct 28, 2021

@rkotze can you review this PR before October 31 or at least add hacktoberfest-accepted label to it? I want to participate in the Hacktoberfest event with it (https://hacktoberfest.digitalocean.com/resources). Thanks!

@rkotze
Copy link
Owner

rkotze commented Oct 29, 2021

Hi @viperet
Thanks for the pr. I've added the label for you.

Just a few things:

  • My Azure CI pipeline was broken, which I've just fixed. Could you update your branch with my changes in 'master' so the CI can run, please?
  • Just got a couple of unit tests failing to fix.

I think my issue was not clear on how I imagined this working.

I see you have allowed sorting per section 'selected', 'unselected', and 'more authors' but what I would prefer is you would toggle and it would apply to all sections rather than per section. How does that sound too you?

Sorry for the confusion.

Copy link
Owner

@rkotze rkotze left a comment

Choose a reason for hiding this comment

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

Please see the previous comment and a couple of unit tests failing to fix.

@viperet
Copy link
Contributor Author

viperet commented Oct 30, 2021

@rkotze sure, will fix that. Do you prefer parameter in the settings or icon in the sidebar to set sorting order?

@rkotze
Copy link
Owner

rkotze commented Oct 30, 2021

@viperet could you make it a setting, please?

@viperet
Copy link
Contributor Author

viperet commented Nov 11, 2021

Yes, sure. I’m on vacation now, will return in a week and change this PR according to your request

@viperet
Copy link
Contributor Author

viperet commented Nov 21, 2021

@rkotze I moved sorting direction to extension configuration and fixed tests.

Copy link
Owner

@rkotze rkotze left a comment

Choose a reason for hiding this comment

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

This looks great 👍 @viperet
One small thing, could you update the readme with information about the setting, please?

@viperet
Copy link
Contributor Author

viperet commented Nov 23, 2021

@rkotze done

Copy link
Owner

@rkotze rkotze left a comment

Choose a reason for hiding this comment

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

Thanks @viperet

@rkotze rkotze merged commit 4d866c7 into rkotze:master Nov 26, 2021
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.

None yet

2 participants