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

feat: 🍰 Alphabetically sorting tags using compute functions on index and more… #3589

Closed
wants to merge 3 commits into from

Conversation

Tirokk
Copy link
Member

@Tirokk Tirokk commented Oct 5, 2020

DriesCruyskens Authored by DriesCruyskens
May 30, 2020
Merged Jun 5, 2020


…-info page. Added test checking tags are present and shown alphabetically.

🍰 Pullrequest

Sorting happens in computed functions, seems like the Vue way to do it. It could also be done once in apollo's update function. Let me know what you think.

Main post page:
image

More info:
image

Issues

Todo

  • None

@Tirokk
Copy link
Member Author

Tirokk commented Oct 7, 2020

cypress[bot] Authored by cypress[bot]
May 30, 2020




Test summary

66 0 0 0


Run details

Project Human-Connection
Status Passed
Commit 38d7ab9
Started Jun 3, 2020 10:43 PM
Ended Jun 3, 2020 11:08 PM
Duration 25:25 💡
OS Linux Ubuntu Linux - 16.04
Browser Firefox 68

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@Tirokk
Copy link
Member Author

Tirokk commented Oct 7, 2020

cypress[bot] Authored by cypress[bot]
May 30, 2020




Test summary

66 0 0 0


Run details

Project Human-Connection
Status Passed
Commit 3526e2154f ℹ️
Started Jun 3, 2020 10:40 PM
Ended Jun 3, 2020 10:59 PM
Duration 18:54 💡
OS Linux Ubuntu Linux - 16.04
Browser Firefox 68

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@@ -143,5 +144,48 @@ describe('PostSlug', () => {
})
})
})

describe('tags shown in tag cloud', () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Tirokk Authored by Tirokk
Jun 3, 2020


Outdated (history rewrite) - original diff


@@ -143,5 +144,48 @@ describe('PostSlug', () => {
         })
       })
     })
+
+    describe('tags shown in tag cloud', () => {

Nicely done 😍
Thanks for testing! 👍🏼

@@ -179,6 +183,9 @@ export default {
if (!author) return false
return this.$store.getters['auth/user'].id === author.id
},
sortedTags() {
Copy link
Member Author

Choose a reason for hiding this comment

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

Tirokk Authored by Tirokk
Jun 3, 2020


Outdated (history rewrite) - original diff


@@ -179,6 +179,20 @@ export default {
       if (!author) return false
       return this.$store.getters['auth/user'].id === author.id
     },
+    sortedTags() {

I would love to have this imported as a more general function located in components/utils/PostHelpers or even in a more general utility file.
This way it can be reused in webapp/pages/post/_id/_slug/more-info.vue. 👇🏼

It can be used to sort the post categories in the future. 🙏🏼
A missing issue as far as I see …

@@ -62,6 +63,9 @@ export default {
post() {
return this.Post ? this.Post[0] || {} : {}
},
sortedTags() {
Copy link
Member Author

Choose a reason for hiding this comment

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

Tirokk Authored by Tirokk
Jun 3, 2020


Outdated (history rewrite) - original diff


@@ -62,6 +62,20 @@ export default {
     post() {
       return this.Post ? this.Post[0] || {} : {}
     },
+    sortedTags() {

Would be nice to have a shared function … see above ☝🏼

@Tirokk
Copy link
Member Author

Tirokk commented Oct 7, 2020

DriesCruyskens Authored by DriesCruyskens
Jun 4, 2020


@Tirokk I moved the sorting function to the PostHelpers file as you suggested.

I could indeed make a new file utils/general.js and make an orderAlphabetically function that accepts the list and a function argument to determine which object property the list should be ordered in (since hashtags and categories have different properties that need to be sorted: id and name respectively). But at that point I feel like a fully-fletched / battle-tested sort function is more in order. Especially if it's going to be used by different people in different context to prevent bugs. Let me know what you think!

Copy link
Contributor

@Mogge Mogge left a comment

Choose a reason for hiding this comment

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

Hey @DriesCruyskens ,
great that you cared for this as well. 💫🚀

Well done! Works very good for me and glad you wrote a test. 😍

Only some little changes I suggested …

Copy link
Contributor

@Mogge Mogge left a comment

Choose a reason for hiding this comment

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

Hey @DriesCruyskens !

I’m graceful that you spend your time again to our project. 🙏🏼
Nice work ! 💫🚀

Genie Counting

I approve!

@Mogge Mogge closed this Oct 8, 2020
@ulfgebhardt ulfgebhardt deleted the pr3589head branch January 7, 2021 13:02
Tirokk pushed a commit that referenced this pull request Jan 18, 2021
…ud-alphabetically

feat: 🍰 Alphabetically sorting tags using compute functions on index and more…
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.

3 participants