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

DIGREPO-807: use FA icons for objects without thumbnails #36

Merged
merged 2 commits into from
Aug 16, 2017

Conversation

dunn
Copy link
Contributor

@dunn dunn commented Aug 14, 2017

No description provided.

position: unset;
width: unset;

a > img {

Choose a reason for hiding this comment

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

Selector should have depth of applicability no greater than 2, but was 6

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah well blame Blacklight for that.

// JIRA: DIGREPO-807
#documents .grid .document .thumbnail {
border: none;
max-width: 100px;

Choose a reason for hiding this comment

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

Properties should be ordered border, max-height, max-width, position, width

// gallery thumbnails
// JIRA: DIGREPO-807
#documents .grid .document .thumbnail {
border: none;

Choose a reason for hiding this comment

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

border: 0 is preferred over border: none

@@ -572,6 +572,24 @@ table#downloads {
// Blacklight
// ----------

// gallery thumbnails
// JIRA: DIGREPO-807
#documents .grid .document .thumbnail {

Choose a reason for hiding this comment

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

Avoid using id selectors
Selector should have depth of applicability no greater than 2, but was 4

id: match[1],
unicodeHex: match[2],
unicodeDec: parseInt(match[2], 16)
})

Choose a reason for hiding this comment

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

Missing semicolon.

function parseIconListFromLess(lines) {
lines = lines.toString();
var match, result = [];
while (match = LESS_VARIABLE_REGEX.exec(lines)) {

Choose a reason for hiding this comment

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

Expected a conditional expression and instead saw an assignment.

unicodeHex: icon.unicodeHex,
unicodeDec: icon.unicodeDec,
data: fontData[icon.unicodeDec].data
}

Choose a reason for hiding this comment

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

Missing semicolon.

return Promise.map(params.sizes, function (siz) {
mkdirp.sync(pathModule.join(params.destFolder, params.color, 'png', siz));
return generatePng(siz, name, params);
}, {concurrency: process.env['JOBS'] || 4});

Choose a reason for hiding this comment

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

['JOBS'] is better written in dot notation.


work.push(Promise.map(iconConfigs, generateIcon, {concurrency: 1}).then(function (done) {
console.log("Done",done.map(function (doneItem) {
return doneItem.color + " " + doneItem.id

Choose a reason for hiding this comment

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

Missing semicolon.

optipng: optipng,
destFolder: destFolder
});
})

Choose a reason for hiding this comment

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

Missing semicolon.

position: unset;
width: unset;

a > img {

Choose a reason for hiding this comment

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

Selector should have depth of applicability no greater than 2, but was 6

@@ -572,6 +572,24 @@ table#downloads {
// Blacklight
// ----------

// override blacklight-gallery
// JIRA: DIGREPO-807
#documents .grid .document .thumbnail {

Choose a reason for hiding this comment

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

Avoid using id selectors
Selector should have depth of applicability no greater than 2, but was 4

@@ -502,7 +502,7 @@ footer {

.document-thumbnail img {
float: right;
max-height: 200px;
max-height: 100px;

Choose a reason for hiding this comment

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

Properties should be ordered float, margin-top, max-height

# The ordering of the field names is the order of the display
config.add_index_field solr_name("work_type_label", :stored_searchable),
label: "Format"

config.add_index_field solr_name("collection_label", :symbol),
label: "Collection"
label: "In Collection"
Copy link
Contributor Author

@dunn dunn Aug 15, 2017

Choose a reason for hiding this comment

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

@chrissyrissmeyer is this OK? I figured since Collections themselves are part of the search results list, saying In Collection would be less ambiguous.

Copy link
Member

Choose a reason for hiding this comment

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

Ran this by Tom B. as well, and he and I both agree that, grammatically, "In Collection" is weird. We both prefer "Collection" and don't feel the meaning of that field it is ambiguous. Additionally, this change would mean that search results labeling would be inconsistent with object view labeling (we use "Collection" on the object view page).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okie doke.

@dunn
Copy link
Contributor Author

dunn commented Aug 15, 2017

New LQQK


screen shot 2017-08-15 at 9 55 06 am


screen shot 2017-08-15 at 9 55 56 am

@@ -175,6 +175,12 @@ def enforce_show_permissions(_opts = {})
config.add_index_field solr_name("issued", :displayable),
label: "Issued Date"

config.add_index_field solr_name("description", :stored_searchable),
label: "Summary",
if: :collection?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Descriptions are often way too long for non-Collections, especially in the case of ETDs.

@@ -1,7 +0,0 @@
<% desc = collection['description_tesim'] %>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Overriding the default partial here is why Collections rendered so differently in search results, with no item number and special formatting.

@dunn
Copy link
Contributor Author

dunn commented Aug 15, 2017

screen shot 2017-08-15 at 10 14 43 am

@dunn
Copy link
Contributor Author

dunn commented Aug 15, 2017

Wait are you kidding me, it uses the thumbnail for the slideshow as well.
screen shot 2017-08-15 at 10 21 30 am

@dunn
Copy link
Contributor Author

dunn commented Aug 15, 2017

I think this is something that needs to be fixed in blacklight-gallery, so that we can use a thumbnail for the pane view and the full-size for the actual slideshow.

@dunn dunn changed the title DIGREPO-807: use icons for Collections and MapSets DIGREPO-807: use FA icons for objects without thumbnails Aug 15, 2017
@dunn
Copy link
Contributor Author

dunn commented Aug 15, 2017

@@ -0,0 +1,20 @@
module Blacklight::GalleryHelper

Choose a reason for hiding this comment

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

Style/FrozenStringLiteralComment: Missing magic comment # frozen_string_literal: true.

@dunn
Copy link
Contributor Author

dunn commented Aug 15, 2017

OK, fixed it to show the larger (400px) images in the slideshow itself:
screen shot 2017-08-15 at 11 34 31 am

@supritasrinivas
Copy link

Oh so sweet! 🍬

render_thumbnail_tag(
document,
options,
url_options.reverse_merge(suppress_link: true)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

url_options should just be options

@dunn dunn merged commit 1708db4 into ucsblibrary:master Aug 16, 2017
@dunn dunn deleted the DIGREPO-807 branch August 16, 2017 20:14
@dunn
Copy link
Contributor Author

dunn commented Aug 30, 2017

For future reference, Blacklight added support for more flexible thumbnail settings:

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