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

[#53429] Project storage menu links does not include prefix. #15028

Merged

Conversation

ba1ash
Copy link
Member

@ba1ash ba1ash commented Mar 18, 2024

@ba1ash ba1ash self-assigned this Mar 18, 2024
@ba1ash ba1ash force-pushed the bug/53429-project-storage-menu-links-does-not-include-prefix branch from 9071c50 to 70ea191 Compare March 18, 2024 18:46
@ba1ash ba1ash marked this pull request as ready for review March 18, 2024 19:36
@ba1ash ba1ash requested a review from a team March 18, 2024 19:36
Copy link
Contributor

@mereghost mereghost left a comment

Choose a reason for hiding this comment

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

LGTM

@ba1ash ba1ash force-pushed the bug/53429-project-storage-menu-links-does-not-include-prefix branch from 70ea191 to 4c7512a Compare March 19, 2024 08:43
Copy link
Member

@akabiru akabiru left a comment

Choose a reason for hiding this comment

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

🟢 Nit I wonder if it's more consistent to define a url_helpers method in ApplicationForm similar to ApplicationComponent#url_helpers to DRY things up a bit as it also seems like an easy miss. Perhaps even a mixin from StaticRouting

Primer ApplicationForm is just Primer::Forms::Base

@mereghost
Copy link
Contributor

Hmmmm good question @akabiru. How often are we using it? Would this change a bunch of the current components? If so I'd make it into a Code Maintenance WP making sure we get all references.

[random unsolicited rant]
I personally hate when pieces of code far from the transport/delivery layer having to know about url_helpers. In this case it is ok as those are components but I've messed with jobs that know how to build application urls. 😠
[/random unsolicited rant]

@akabiru
Copy link
Member

akabiru commented Mar 19, 2024

code far from the transport/delivery layer having to

By my reckoning we're not using it too much- I guess here I'm looking out for the probability of repeating the same "mistake". 😄 I can see myself or someone new to the team defaulting to Rails.application.routes again. Perhaps the larger question is whether Rails.application.routes should be then one that is prefix aware and not a completely separate module. The ol' convention vs configuration approach

This can be a code maintenance discussion for sure

@ba1ash ba1ash merged commit 31970ee into dev Mar 19, 2024
9 checks passed
@ba1ash ba1ash deleted the bug/53429-project-storage-menu-links-does-not-include-prefix branch March 19, 2024 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants