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

Improvements to suggested and archive URLs when editing a mapping [60838740] #83

Merged
merged 11 commits into from Nov 25, 2013

Conversation

@fofr
Copy link
Contributor

@fofr fofr commented Nov 22, 2013

  • Archive and suggested URLs are secondary fields that rarely need to be filled in, move them behind a disclosure to avoid confusion
  • Introduce text that explains the meaning of both URL types, with appropriate link text to disclose the fields
  • Use cancel or remove to switch the view back and reset the field
  • Default to showing the fields when they are present
  • Switch labels for read only fields to a key-value definition list that simulates label styles
  • Introduce a mappings helper to create national archive URLs
  • When JS is disabled, show all form values and separate the form with visible legends, and hide JS interactions
  • There is a known caveat that if the default host is wrong the national archive link may not work and that the fix isn’t clear to the user
  • Cleanup some of the base styles to use more of the bootstrap defaults
  • Change how we deal with the non-javascript case so we have hooks for JS enabled and disabled
Paul Hayes added 9 commits Nov 19, 2013
* Use larger bootstrap defaults
* Make the date human readable
* Return a copyable version that can be pasted into a National archive
URL
* Refactor the redirect-to-link class for wider use
* Add to the style guide
* We can use bootstrap’s mute instead
* Avoid repaints by toggling JS class before CSS has loaded
* Use a class for enabled and disabled
* Use minified inline JavaScript instead
* This also mimics the Modernizr approach
* Separate into sections with comments
* Include content toggle classes
* Remove body and pre styles that aren’t being used, or are met by
bootstrap defaults
* Archive and suggested URLs are secondary fields that rarely need to
be filled in, move them behind a disclosure to avoid confusion
* Introduce text that explains the meaning of both URL types, with
appropriate link text to disclose the fields
* Use cancel or remove to switch the view back and reset the field
* Default to showing the fields when they are present
* Switch labels for read only fields to a key-value definition list
that simulates label styles
* Introduce a mappings helper to create national archive URLs
* When JS is disabled, show all form values and separate the form with
visible legends, and hide JS interactions
* Behaviour when creating mapping still TBD
* There is known caveat that if the default host is wrong the national
archive link may not work and that the fix isn’t clear to the user
* We can’t show a national archive link until the mapping path is
canonicalised
* Hide the TNA link and alternative field
* We are now using bootstrap default font-size and line-height
@jamiecobbett
Copy link
Contributor

@jamiecobbett jamiecobbett commented Nov 22, 2013

This looks awesome, good work :)

@@ -54,6 +54,21 @@ def mapping_edit_tabs(options = {})
end

##
# Generate national archive index URL
def national_archive_index_url(mapping)

This comment has been minimized.

@jamiecobbett

jamiecobbett Nov 22, 2013
Contributor

I wonder if these two methods really belong in the model layer? Two reasons:

  • I think it is probably a model concern
  • It would be good to eventually share this code with bouncer, and sharing models (via a gem) is probably how we'd do that

This comment has been minimized.

@jamiecobbett

jamiecobbett Nov 22, 2013
Contributor

After a chat with @rgarner and @fofr, he's going to move these methods to the model.

This comment has been minimized.

@fofr

fofr Nov 25, 2013
Author Contributor

Methods moved.

@@ -9,5 +9,5 @@
<dd><%= site.query_params.presence || 'N/A' %></dd>

<dt><abbr title="The National Archive">TNA</abbr> timestamp</dt>
<dd><%= site.tna_timestamp %></dd>
<dd><%= site.tna_timestamp.to_formatted_s(:long) %> (<%= site.tna_timestamp.to_formatted_s(:number) %>)</dd>

This comment has been minimized.

@jamiecobbett

jamiecobbett Nov 22, 2013
Contributor

👍

This comment has been minimized.

@jamiecobbett

jamiecobbett Nov 22, 2013
Contributor

If tna_timestamp is nil, this will prevent the page from rendering. For this reason, I avoid adding optional fields to (default) factories as you have done, and instead override them when I need them in a test.

Having said that, I'm going to create a ticket to make tna_timestamp NOT NULL and review other fields, so this change is fine, but my thoughts on optional fields in factories stands.

This comment has been minimized.

@fofr

fofr Nov 22, 2013
Author Contributor

That logic is sound, and I'll follow it from now on. Also, 👍 on raising that new ticket.

@@ -55,8 +55,8 @@
<td>
<%= example_url(mapping) %>
<% if mapping.redirect? %>
<span class="redirects-to">redirects to</span><br>
<%= link_to mapping.new_url, mapping.new_url, class: 'redirects-to-link' if mapping.new_url.present? %>
<i class="muted">redirects to</i><br>

This comment has been minimized.

@rgarner

rgarner Nov 22, 2013
Contributor

Wow. i is back as 'alternative voice or mood'. i literally had no idea.

Paul Hayes added 2 commits Nov 25, 2013
* Replace example_url, national_archive_url and
national_archive_index_url
* Use a terser test format
rgarner added a commit that referenced this pull request Nov 25, 2013
Improvements to suggested and archive URLs when editing a mapping [60838740]
@rgarner rgarner merged commit d76b806 into master Nov 25, 2013
@rgarner rgarner deleted the edit-mapping-improvements branch Nov 25, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

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