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

Combine index menu option definitions #870

Closed
2 tasks done
andrewtavis opened this issue May 3, 2024 · 15 comments
Closed
2 tasks done

Combine index menu option definitions #870

andrewtavis opened this issue May 3, 2024 · 15 comments
Assignees
Labels
feature New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@andrewtavis
Copy link
Member

andrewtavis commented May 3, 2024

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 😊

@andrewtavis andrewtavis added feature New feature or request help wanted Extra attention is needed good first issue Good for newcomers labels May 3, 2024
@tasawar-hussain
Copy link

I would like to take it up!

@andrewtavis
Copy link
Member Author

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
Copy link
Member Author

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

@tasawar-hussain
Copy link

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

@andrewtavis
Copy link
Member Author

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

@t3azr
Copy link
Contributor

t3azr commented Jul 9, 2024

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
Copy link
Member Author

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

@andrewtavis
Copy link
Member Author

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
Copy link
Contributor

t3azr commented Jul 9, 2024

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
Copy link
Member Author

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
Copy link
Contributor

t3azr commented Jul 17, 2024

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
Copy link
Member Author

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
Copy link
Contributor

t3azr commented Jul 28, 2024

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

@andrewtavis
Copy link
Member Author

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
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
Archived in project
Development

No branches or pull requests

3 participants