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 link to show resource from collection table #727

Merged
merged 1 commit into from
Jan 9, 2017
Merged

Conversation

odlp
Copy link
Contributor

@odlp odlp commented Jan 6, 2017

Adds a link to show the resource within each cell of the collection table, as an alternative to the data-url on the tr. The link is styled with the default text colour so there's no visual change.

This allows a resource to be shown without javascript enabled (and also facilitates faster tests using RackTest).

On my machine running the same test comparing:

  • click_show_link_for (+ no JS)
  • click_row_for (+ with JS)

is a lot faster, as expected:

Run no JS JS
1 0.88 2.04
2 0.85 1.93
3 0.85 1.96
4 0.84 2.02
5 0.82 2.04
Avg 0.85 2.00
Std.Dev 0.02 0.05

(in seconds, running each test independently, excluding file load times)

Allows a resource to be shown without javascript enabled
(using "data-url"). Also facilitates faster tests using RackTest.
@@ -22,6 +22,15 @@
expect(page).to have_content(customer.email)
end

it "links to the customer show page without javascript", js: false do
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this means that now we'll be able to disable JavaScript by default and enable JS instead?

Copy link
Contributor Author

@odlp odlp Jan 6, 2017

Choose a reason for hiding this comment

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

I think the default driver is currently RackTest for most of the feature specs. The js: false is redundant but I put it there to be explicit.

11 / 56 feature specs enable javascript with the :js flag:

bundle exec rspec spec/features
# 56 examples, 0 failures

bundle exec rspec spec/features --tag js
# 11 examples, 0 failures

We could convert these to use the hyperlink, but it's quite handy to have the coverage & confidence the data-url / JS option still works.

What do you think @nickcharlton?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh sorry... re-reading your comment. Do you mean invert the test names? So links to the customer show page is the vanilla version, then links to the customer show page with javascript or similar?

@nickcharlton
Copy link
Member

I like it! :shipit:

@odlp odlp merged commit f3d9c6c into master Jan 9, 2017
@odlp odlp deleted the op-add-show-link branch January 9, 2017 10:20
fwolfst pushed a commit to fwolfst/administrate that referenced this pull request Mar 8, 2017
Allows a resource to be shown without javascript enabled
(which uses "data-url" attrs). Also facilitates faster tests using RackTest.
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