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

Replace print pdf icon #2267

Merged
merged 2 commits into from
Jul 21, 2021
Merged

Replace print pdf icon #2267

merged 2 commits into from
Jul 21, 2021

Conversation

SteelWagstaff
Copy link
Member

@SteelWagstaff SteelWagstaff commented Jul 20, 2021

This PR fixes #2264 and #2266

To test:

  1. Produce a Print PDF export
  2. Check to see that the new icon appears on the export page. The icon should have a red banner and should include the text 'Print PDF'

Note: I did not replace the 36 and 72 px versions of this image as they are not used anywhere in Pressbooks. The large size is explicitly specified for the export table, which is the only place these icons are currently used in Pressbooks:

protected function getIcon( $file, $size = 'large' ) {
$file_class = $this->getCssClass( $file );
$html = "<div class='export-file-icon {$size} {$file_class}' title='" . esc_attr( $file ) . "'></div>";

@codecov
Copy link

codecov bot commented Jul 20, 2021

Codecov Report

Merging #2267 (d0a0b74) into dev (86b921b) will increase coverage by 0.07%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##                dev    #2267      +/-   ##
============================================
+ Coverage     68.40%   68.47%   +0.07%     
+ Complexity     4944     4928      -16     
============================================
  Files           132      131       -1     
  Lines         21363    21353      -10     
============================================
+ Hits          14613    14622       +9     
+ Misses         6750     6731      -19     

@SteelWagstaff
Copy link
Member Author

@greatislander Is there a historical reason why we need to leave the small sizes of export icons SCSS

&.small {
width: 36px;
height: 36px;
background-size: 36px 36px;
background-position: -0 0;
background-repeat: no-repeat;
&.epub {
background-image: url(../images/epub-36.png);
@include hidpi {
background-image: url(../images/epub-72.png);
}
}
&.epub3 {
background-image: url(../images/epub3-36.png);
@include hidpi {
background-image: url(../images/epub3-72.png);
}
}
&.icml {
background-image: url(../images/icml-36.png);
@include hidpi {
background-image: url(../images/icml-72.png);
}
}
&.mobi {
background-image: url(../images/mobi-36.png);
@include hidpi {
background-image: url(../images/mobi-72.png);
}
}
&.print-pdf {
background-image: url('../images/print-pdf-36.png');
@include hidpi {
background-image: url('../images/print-pdf-72.png');
}
}
&.pdf {
background-image: url(../images/pdf-36.png);
@include hidpi {
background-image: url(../images/pdf-72.png);
}
}
&.wxr {
background-image: url(../images/wxr-36.png);
@include hidpi {
background-image: url(../images/wxr-72.png);
}
}
&.vanillawxr {
background-image: url(../images/vanillawxr-36.png);
@include hidpi {
background-image: url(../images/vanillawxr-72.png);
}
}
&.xhtml {
background-image: url(../images/xhtml-36.png);
@include hidpi {
background-image: url(../images/xhtml-72.png);
}
}
&.htmlbook {
background-image: url(../images/html5-36.png);
@include hidpi {
background-image: url(../images/html5-72.png);
}
}
&.odt {
background-image: url(../images/odt-36.png);
@include hidpi {
background-image: url(../images/odt-72.png);
}
}
&.weblinks {
background-image: url(../images/imscc-36.png);
@include hidpi {
background-image: url(../images/imscc-72.png);
}
}
}
and the corresponding image assets in Pressbooks?

fdalcin
fdalcin previously approved these changes Jul 21, 2021
Copy link
Contributor

@fdalcin fdalcin left a comment

Choose a reason for hiding this comment

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

@greatislander
Copy link
Contributor

@greatislander Is there a historical reason why we need to leave the small sizes of export icons SCSS and the corresponding image assets in Pressbooks?

@SteelWagstaff Nope. It used to be that the exports were displayed in a grid, with the most recent of each format as large icons and then a more compact grid with smaller icons below. These can and should be removed.

@SteelWagstaff
Copy link
Member Author

@fdalcin Just pushed a commit which removes the unused small sizes of file export icons. If it still looks good to you, please squash and merge at your convenience.

@fdalcin fdalcin merged commit 021f345 into dev Jul 21, 2021
@fdalcin fdalcin deleted the pb-2264-print-pdf-icon branch July 21, 2021 14:02
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.

Print PDF icon image missing on export page
3 participants