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

Make tabs on editions linkable, and work without js #343

merged 9 commits into from Jan 6, 2015


Copy link

@fofr fofr commented Jan 2, 2015

  • Add routes for each edition tab
  • Create a tab model for generating tab links, titles and for determining which tab is active
  • Update the page URL when selecting tabs with Javascript
  • Show the correct tab when going directly to a tab url, eg /metadata
  • Toggle tabs when clicking on them whether using javascript or not
  • Include Bootstrap recommended aria attributes


fofr added 7 commits Dec 30, 2014
Bootstrap tabs work best when they are a direct child (>) of the
tab-content container
* Make tabs addressable
* Link tabs to pages so they can be opened without js, or in a new tab
or window
* Use replaceState to update the URL when changing tabs, while avoiding
adding the tab to the browser history stack (maintaining current
navigation behaviour)
* Simplify view
* Go through the “show” method, rather than just rendering that view
* Form needs to have “edition-form” ID for javascript to work, which
was causing tests to fail
* Putting the modals from `edition_activities_fields` within the tab
means they are hidden if the tab isn’t selected, which breaks current
* Bootstrap tabs don’t work correctly if the tab-pane isn’t nested
correctly. A shim is needed.
* Tab doesn’t need to know anything about editions, routes or partials
* Test tab model
Check that the correct tabs show
* when clicking on them with or without javascript
* when visiting them directly
* when saving an edition

Tests cover javascript shim in tab_switcher.js, and the need to define
the tabs/active tab when returning from a failed edition save
Copy link

@vinayvinay vinayvinay commented Jan 5, 2015

@fofr: i like this change. i've extracted the view logic from the EditionsController into a TabsHelper and moved the View::Edition::Tab into the TabsHelper because it is a view layer object, as opposed to a model object that represents something stored in the database. these changes are on a commit on this branch, so if you like that change i can send you a diff, or your could cherry-pick that commit; whatever you're comfortable with.

@vinayvinay vinayvinay self-assigned this Jan 5, 2015
Copy link
Contributor Author

@fofr fofr commented Jan 5, 2015

The view model is a pattern I've borrowed from Transition, as created by @rgarner and @jennyd. There it's used for filters, hits categories and time periods. These models begun life as helpers.


@rgarner Do you have an opinion on


This comment has been minimized.

Copy link

@rgarner rgarner commented on app/models/view/edition/tab.rb in 9495098 Jan 5, 2015

I might consider a heredoc here; it's getting a little large and noisy, and losing \ from all the \" would help. How about

      <a href="#{path(edition_path)}" 


This comment has been minimized.

Copy link

@bradwright bradwright replied Jan 5, 2015

I'd rather we didn't construct html_safe strings inside classes at all - could we use a builder here instead to ensure things are appropriately escaped?

This comment has been minimized.

Copy link

@rgarner rgarner replied Jan 5, 2015

Good point. I'm making a rather large (and entirely invalid) assumption that we trust title and path(edition_path).

Paul Hayes and Vinay Patel and others added 2 commits Jan 5, 2015
`View::Edition::Tab` is a strictly view layer concern. Hence, extracting
the logic of rendering the active tab from the controller into TabsHelper
and renaming `View::Edition::Tab` to `TabsHelper::Edition::Tab`.
@fofr fofr force-pushed the linkable-tabs branch from 7daefe7 to 15e0c70 Jan 6, 2015
vinayvinay added a commit that referenced this pull request Jan 6, 2015
Make tabs on editions linkable, and work without js
@vinayvinay vinayvinay merged commit bb0b9b8 into master Jan 6, 2015
1 check passed
1 check passed
default "Build #569 succeeded on Jenkins"
@vinayvinay vinayvinay deleted the linkable-tabs branch Jan 6, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.