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

Add current_page? helper for pages. #1074

Merged
merged 5 commits into from
Apr 2, 2020
Merged

Add current_page? helper for pages. #1074

merged 5 commits into from
Apr 2, 2020

Conversation

wout
Copy link
Contributor

@wout wout commented Mar 31, 2020

Purpose

Implementing #1073.

Description

This adds the current_page? helper with similar functionality like the one in Rails. Although, it does not cover all Rails' cases. What it does not do:

  1. handle cases with double escaped params
  2. handle cases with different encoding
  3. test against a full uri, including protocol and port

I'm not quite sure if they are relevant or even possible. Looking at the available data in a request, I wonder if it is even feasible to implement point 3. There is no notion of the protocol, but even then, would we even need to test the protocol?

Checklist

  • - An issue already exists detailing the issue/or feature request that this PR fixes
  • - All specs are formatted with crystal tool format spec src
  • - Inline documentation has been added and/or updated
  • - Lucky builds on docker with ./script/setup
  • - All builds and specs pass on docker with ./script/test

src/lucky/page_helpers/url_helpers.cr Outdated Show resolved Hide resolved
src/lucky/page_helpers/url_helpers.cr Outdated Show resolved Hide resolved
src/lucky/page_helpers/url_helpers.cr Outdated Show resolved Hide resolved
Copy link
Member

@jwoertink jwoertink left a comment

Choose a reason for hiding this comment

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

Hey, thanks for taking a look at this! I just left a comments.

src/lucky/page_helpers/url_helpers.cr Outdated Show resolved Hide resolved
src/lucky/page_helpers/url_helpers.cr Outdated Show resolved Hide resolved
src/lucky/page_helpers/url_helpers.cr Show resolved Hide resolved
@wout
Copy link
Contributor Author

wout commented Apr 1, 2020

Just pushed an updated version where host with port are tested as well if the given value is a full uri. For example:

# Visiting https://example.com/action
view.current_page?("https://example.com/action")
# # => true
# Visiting https://example.io/action
view.current_page?("https://example.com/action")
# # => false

But:

# Visiting https://example.com/action
view.current_page?("http://example.com/action")
# # => true

Might be confusing.

UPDATE

As discussed in #1073, we'll leave out the schema since https is a requirement these days.

Copy link
Member

@paulcsmith paulcsmith left a comment

Choose a reason for hiding this comment

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

Really wonderful stuff. Just a couple specs for the RouteHelper version. Overall looks fantastic. And thanks for adding docs

src/lucky/page_helpers/url_helpers.cr Outdated Show resolved Hide resolved
spec/lucky/url_helpers_spec.cr Show resolved Hide resolved
spec/lucky/url_helpers_spec.cr Show resolved Hide resolved
spec/lucky/url_helpers_spec.cr Show resolved Hide resolved
@wout
Copy link
Contributor Author

wout commented Apr 2, 2020

I assumed this was good to go, but just thought of something. The query params probably need to be unescaped before testing against the given value.

@wout
Copy link
Contributor Author

wout commented Apr 2, 2020

Ok, this is the last bit I think. Now uri encoded query params are properly decoded before testing.

Copy link
Member

@paulcsmith paulcsmith left a comment

Choose a reason for hiding this comment

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

Looks perfect! Nice catches and great specs

@paulcsmith paulcsmith merged commit 685e0ca into luckyframework:master Apr 2, 2020
@wout wout deleted the 1073-current-page-helper branch April 3, 2020 09:10
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.

3 participants