search

activist-org / activist

An open-source activism platform
https://activist.org
GNU Affero General Public License v3.0
225 stars 185 forks source link

Combine index menu option definitions #870

Closed andrewtavis closed 1 month ago

andrewtavis commented 5 months ago

Terms

Description

The definitions for index menu options were copied in some cases, and it would be great if they could be combined. Specifically on the following two pages:

we have organizationButtons and eventButtons respectively that should likely be using the logic in useMenuEntriesState.

Contribution

Would be great if someone wanted to help with this! Happy to assist as needed or get to it myself eventually 😊

tasawar-hussain commented 5 months ago

I would like to take it up!

andrewtavis commented 5 months ago

Thanks for checking the issues and writing in so many, @tasawar-hussain! Assigning you here, and please let us know if there's something we can do to help!

andrewtavis commented 4 months ago

Hey @tasawar-hussain 👋 Checking in with you here :) Is there something we can do to support?

tasawar-hussain commented 4 months ago

Nothing right now, will start this week, and get back to you if there is something

andrewtavis commented 4 months ago

Sounds great, @tasawar-hussain :) Looking forward to it!

t3azr commented 2 months ago

Hello!! I'd like to try at this if that's okay : ). If I'm understanding/reading it right -- in both the pages, the organisation menu buttons are being manually created, and instead you want it to be done through useMenuEntriesState? Please feel free to correct me // add anything on !!

andrewtavis commented 2 months ago

Yes, exactly that, @t3azr! Thanks so much for your willingness to work on this! :)

andrewtavis commented 2 months ago

Give the contribution guide a Quick Look for the sections you'll need for this, and please let us know if there's anything you can do to help!

t3azr commented 2 months ago

could i just ask , in the pages provided , i can only see organisedButtons , no eventButtons . did the original post mean to also change it to eventButtons in the event page or am i missing something ? :)

andrewtavis commented 2 months ago

Hey @t3azr 👋 There's a chance that the original eventButtons got deleted, but we can take a look at this later. Do you want to implement for organizationButtons and we can check further in the PR once that's ready? :)

t3azr commented 2 months ago

Hello !! I've gotten most of it done now, I think. Just final checks. However: I came across something and I can't tell if I've misunderstood the code or if it's a bug.

When menu entries are created it Numbers() the route value to create the ID. However, for projects and events, the route is an alphanumeric string - so this was outputting NaN. This was causing issues as the routeURL and such use that ID to create the full link/route.

To kind of bandaid this, I've just added an intermediary idStr, which is the same as the ID, but without the Number(), as I didn't want to remove it entirely in case it's important/used elsewhere. So the routeURL uses idStr instead of the normal id.

However: I'm not sure where else this function is used, so I wanted to make you aware of it in case it breaks any other sites where Number() is needed!!

The ID for each entry is left as NaN, and afaik, this hasn't caused any issues -- the buttons still redirect to its respective page. Thanks!! Also, just a P.S : ignore my question about eventButtons, I had only quickly skimmed the code and completely missed it -- it is there !!

andrewtavis commented 2 months ago

Hey @t3azr 👋 Really sorry for the delay here. I've been traveling/seeing friends and family and just barely getting to critical issues and reviews. I do have this in mind, and am on my way back home today!

Hope all's well :)

t3azr commented 2 months ago

thanks for your response ! no worries - i’ve just been on holiday as well :) hope all is well for you too !

andrewtavis commented 2 months ago

To kind of bandaid this, I've just added an intermediary idStr, which is the same as the ID, but without the Number(), as I didn't want to remove it entirely in case it's important/used elsewhere. So the routeURL uses idStr instead of the normal id.

However: I'm not sure where else this function is used, so I wanted to make you aware of it in case it breaks any other sites where Number() is needed!!

Am doing well, thanks :) In regards to this, I'm very happy to check out a PR with these changes and can do any cleanup that's needed 😊 Thanks so much for detailing what you're changing!

andrewtavis commented 1 month ago

Closed by #940 :) Thanks so much for the work here, @t3azr! Really appreciate it :) Let us know if another issue sounds appealing!