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

Adding imagelayers link to search results #532

Merged
merged 1 commit into from
Jun 18, 2015

Conversation

rheinwein
Copy link
Collaborator

Adding imagelayers icon next to remote images in both the main search results as well as the modal. Behavior is the same as on the image list page.
image

@@ -12,7 +12,8 @@
status_label: 'Local',
badge_class: 'local',
star_count: 123,
docker_index_url: 'index.docker/boom/shaka'
docker_index_url: 'index.docker/boom/shaka',
imagelayers_url: 'imagelayers.io/?images=boom/shaka'
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about
imagelayers_url: "#{IMAGELAYERS_URL}?images=boom/shaka"

@dharmamike
Copy link
Collaborator

👍

@rheinwein rheinwein force-pushed the feature/imagelayers-search-integration branch from adf0dc7 to 2ea1734 Compare June 18, 2015 15:45
@argvader
Copy link
Collaborator

👍

@rheinwein rheinwein force-pushed the feature/imagelayers-search-integration branch from 2ea1734 to 667576a Compare June 18, 2015 16:41
@rheinwein rheinwein merged commit 667576a into master Jun 18, 2015
@rheinwein rheinwein deleted the feature/imagelayers-search-integration branch June 18, 2015 16:44
@@ -39,6 +39,15 @@ def docker_index_url
end
end

def imagelayers_url
if remote?
Copy link
Contributor

Choose a reason for hiding this comment

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

Not that big of a deal, but it would probably be better to put these implementations into the child classes that are inheriting from this base. The example set above (line 36) is a bad one, that creates a bi-directional dependency... This super class shouldn't really have knowledge of the child classes. There's also a missing test for the remote image case, I think. Though it gets tested implicitly in the as_json tests...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I had the same thought, though wanted to stay with the existing pattern we use for the hub url. I'll refactor when I add the IL links to the application page (and add that missing test).

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.

4 participants