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

Don't hardcode URLs in Javascript #2282

Merged
merged 1 commit into from
Apr 11, 2022
Merged

Don't hardcode URLs in Javascript #2282

merged 1 commit into from
Apr 11, 2022

Conversation

dssjoblom
Copy link

PR for #2275 as requested.

@tvdeyen
Copy link
Member

tvdeyen commented Apr 6, 2022

@dssjoblom thanks. Would you mind to add a spec and a proper PR title (maybe copy the commit message), because this will end up in the change log.

@dssjoblom dssjoblom changed the title PR for #2275 #2275 - Don't hardcode URLs in Javascript Apr 7, 2022
@dssjoblom dssjoblom changed the title #2275 - Don't hardcode URLs in Javascript Don't hardcode URLs in Javascript Apr 7, 2022
@dssjoblom
Copy link
Author

@tvdeyen I changed the title, but I think writing a good spec for this is a bit beyond my capabilities. Ideally, the test would:

  1. Mount Alchemy::Engine on a custom path e.g. /prefix
  2. In Capybara, navigate to a specific page editor that contains a picture (e.g. prefix/admin/pages/1/edit)
  3. In Capybara, click on the "change image" icon, select an image, and then press save

This test should work without any issues (the earlier code only worked if Alchemy was mounted on '/').

@tvdeyen
Copy link
Member

tvdeyen commented Apr 7, 2022

@tvdeyen I changed the title, but I think writing a good spec for this is a bit beyond my capabilities. Ideally, the test would:

  1. Mount Alchemy::Engine on a custom path e.g. /prefix
  2. In Capybara, navigate to a specific page editor that contains a picture (e.g. prefix/admin/pages/1/edit)
  3. In Capybara, click on the "change image" icon, select an image, and then press save

This test should work without any issues (the earlier code only worked if Alchemy was mounted on '/').

I was thinking about a view spec for the alchemy/admin/_routes partial. Everything else would be overkill.

Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

Since we do not have any specs for the view for other routes, I think it is fine to have any for this change.

@tvdeyen tvdeyen merged commit 469301b into AlchemyCMS:main Apr 11, 2022
@tvdeyen tvdeyen added this to the 6.0 milestone Apr 11, 2022
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