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

Space: BYO Domains drop Space from polymorphic_urls #1000

Merged
merged 3 commits into from
Dec 21, 2022

Conversation

zspencer
Copy link
Member

#74

This had been bugging me for a bit, where the paths on the page would always include the spaces, even if they were being routed to a custom domain.

Now, we send folks to the domain that the space claims and drop the Space from the path, but only on urls built using Rails Polymorphic url builders.

For example:

space_room_path(space, room) will still go to
current_domain.example.com/spaces/:space_id/rooms/:room_id, while url_for([space, room]) or link_to([space, room]) will go to
space_domain.example.com/rooms/:room_id

#74

This had been bugging me for a bit, where the paths on the page would
always include the spaces, even if they were being routed to a custom
domain.

Now, we send folks to the domain that the space claims and drop the
Space from the path, but only on urls built using Rails Polymorphic url
builders.

For example:

`space_room_path(space, room)` will still go to
`current_domain.example.com/spaces/:space_id/rooms/:room_id`, while `url_for([space, room])` or
`link_to([space, room])` will go to
`space_domain.example.com/rooms/:room_id`
@zspencer zspencer changed the title Space: Branded Domains drop Space from polymorphic_urls Space: BYO Domains drop Space from polymorphic_urls Dec 18, 2022
Move (almost) everything to the Polymorphic syntax!
Copy link
Member

@anaulin anaulin left a comment

Choose a reason for hiding this comment

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

Thank you for improving this! ™️ 🔗

I left some minor non-blocking comments below.

options
end

if space
Copy link
Member

Choose a reason for hiding this comment

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

You could return early unless space.present?, I think it would be a tad more readable

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I need to return super unless space but ya!

@@ -27,6 +27,38 @@ class ApplicationController < ActionController::Base

helper_method :current_person

helper_method def url_for(options)
Copy link
Member

Choose a reason for hiding this comment

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

These two methods are arcane enough that I think they warrant at least a one-sentence comment explaining that they augment the standard Rails helpers to make URLs on branded domains nicer.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call, I've linked to the core docs and included a sentance.

options.delete_at(0)
elsif [:edit, :new].include?(options[0]) && options[1].is_a?(Space) && options[1].branded_domain.present?
options.delete_at(1)
elsif options.is_a?(Space) && options.branded_domain.present?
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if it is worth it, or if it would make the code even harder to understand, but these checks for "is this a Space and does it have a branded domain" could be tightened up like:

thing_were_checking.try(:branded_domain).present?

Which is a little bit more flexible (will work for any object that has a branded_domain, even if it is not a Space), but I think works for our purposes.

Copy link
Member Author

Choose a reason for hiding this comment

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

I always forget about .try(:foo)!

@zspencer zspencer merged commit f7234cc into main Dec 21, 2022
@zspencer zspencer deleted the spaces/drop-space-from-url-on-custom-domains branch December 21, 2022 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants