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

Allows the File Set Show page to display. #1476

Merged
merged 1 commit into from
Dec 16, 2015
Merged

Allows the File Set Show page to display. #1476

merged 1 commit into from
Dec 16, 2015

Conversation

hectorcorrea
Copy link
Contributor

There are still several TODOs that need to be addressed. Fixes #1464

This is how it looks so far.
screen shot 2015-12-15 at 10 20 02 am

@mjgiarlo
Copy link
Member

@hectorcorrea Not sure if this helps document/break down your TODOs, but you can construct task lists in GitHub comments like so:

  • Do a thing
  • Frobulate the widgetizers
  • Burn rubocop to the ground
  • Cuss @mjgiarlo out on GitHub

(It's also cool to have TODOs in code comments, naturally.)

@@ -15,12 +15,20 @@ def render_collection_list(fs)
end

def display_multiple(value)
auto_link(value.join(" | "))
# TODO: figure out why sometimes we get an string and some others an array
Copy link
Member

Choose a reason for hiding this comment

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

This is a good TODO to leave in. I think the next few lines of code are a good candidate to take a "tell, don't ask" approach. Like so:

string_value = Array(value).join(" | ")
auto_link(string_value)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No changes were needed to this method after all.

@mjgiarlo
Copy link
Member

Looking good so far, @hectorcorrea! Left some comments about the nitty-gritties.

@hectorcorrea
Copy link
Contributor Author

This is ready to merge. Missing items have been entered as new tickets (See links above)

@@ -15,16 +15,16 @@ def render_collection_list(fs)
end

def display_multiple(value)
auto_link(value.join(" | "))
auto_link(value)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

hum...hold on one sec. I shouldn't have changed this. Resubmitting in a second.

<p class="genericfile_description"><%= display_multiple @presenter.description %></p>
<%= render 'show_descriptions' %>
Copy link
Member

Choose a reason for hiding this comment

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

Are you deleting this because there's currently no way to edit the descriptions that would be shown here? If so, I'd say we should use <%# TODO: render 'show_descriptions' %> instead, since there are discussions about allowing FileSet descriptions in Sufia 7. (If not in the 7.0.0 release.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member

Choose a reason for hiding this comment

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

👏

@mjgiarlo
Copy link
Member

Hi @hectorcorrea, I have just one outstanding question about a partial rendering deleted in the FS show view.

mjgiarlo added a commit that referenced this pull request Dec 16, 2015
Allows the File Set Show page to display.
@mjgiarlo mjgiarlo merged commit 60326e5 into master Dec 16, 2015
@mjgiarlo mjgiarlo deleted the file_set_show branch December 16, 2015 19:38
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