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

Separate local from imported metadata properties for ScannedResources #412

Merged
merged 2 commits into from
Oct 20, 2017

Conversation

jrgriffiniii
Copy link
Contributor

Resolves #331

@jrgriffiniii jrgriffiniii force-pushed the issues-331-jrgriffiniii-improve-metadata-display branch 3 times, most recently from 7fc2da3 to 4303165 Compare October 19, 2017 20:37
@jrgriffiniii jrgriffiniii changed the title [WIP] Separate local from imported metadata properties for ScannedResources Separate local from imported metadata properties for ScannedResources Oct 19, 2017
# @param attribute_names [Symbol]
def self.suppress(attribute_names)
attribute_names = Array.wrap(attribute_names)
attribute_names.each { |attribute_name| self.displayed_attributes.delete(attribute_name) }
Copy link
Member

Choose a reason for hiding this comment

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

I think several of these could be shortened, e.g. couldn't this be:

self.displayed_attributes -= Array.wrap(attribute_names)

@jrgriffiniii jrgriffiniii changed the title Separate local from imported metadata properties for ScannedResources [WIP] Separate local from imported metadata properties for ScannedResources Oct 20, 2017
@@ -0,0 +1,28 @@
<div id="attributes">
Copy link
Member

Choose a reason for hiding this comment

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

Can we avoid some of these duplicate views? Could we make one of them _resource_attributes_default.html.erb? Could we have one copy and then have this view and the other one call out to it with render partial ?

@escowles
Copy link
Member

I like the general approach here, and think this is a big improvement on making the logic for what to display clearer. I left a couple of comments around streamlining a couple of things.

…tes; Separating imported metadata attributes from local attributes within the ScannedResource and ScannedMap Decorators
…ucturing the view templates to use a partial for imported attributes
@jrgriffiniii jrgriffiniii force-pushed the issues-331-jrgriffiniii-improve-metadata-display branch from 4303165 to 885d2bd Compare October 20, 2017 18:11
@jrgriffiniii jrgriffiniii changed the title [WIP] Separate local from imported metadata properties for ScannedResources Separate local from imported metadata properties for ScannedResources Oct 20, 2017
Copy link
Member

@escowles escowles left a comment

Choose a reason for hiding this comment

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

👍 looks good!

@escowles escowles merged commit 1612966 into master Oct 20, 2017
@escowles escowles deleted the issues-331-jrgriffiniii-improve-metadata-display branch October 20, 2017 18:51
@escowles escowles removed the Review label Oct 20, 2017
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.

None yet

2 participants