-
Notifications
You must be signed in to change notification settings - Fork 145
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
overview page for series available in a press to be added OMP #855
base: main
Are you sure you want to change the base?
Conversation
adding a page in OMP that displays the available book series.
adding the overview page for the series: seriesIndex.tpl. it was noted that series.tpl would semantically more beautiful but that filename is already taken.
edit facilitates an index page for series
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @gemusehandler! Nice work. I've added some comments to the code to reflect some minor syntax and whitespace changes in order to fit our coding style.
I've also provided a commit that just re-arranges your code a little bit so that the series index can be found a /catalog/series
instead of /catalog/seriesIndex
. You can see that at gemusehandler#1 and you should be able to just merge it into your code.
</div> | ||
|
||
<div class="metadata"> | ||
<h3><a href="{url router=$smarty.const.ROUTE_PAGE page="catalog" op="series" path=$browseSeriesItem->getPath()|escape}"> {$browseSeriesItem->getLocalizedFullTitle()|escape}</a></h3> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change h3
to h2
to reflect the page structure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in that case the h2 at the start of the page needs to become a h1
I understand the reasoning that the larger heading at a page is h1,
but from a site perspective we could have a different view.
I do as suggested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, good catch. The page title must be in <h1>
.
Thanks for that.
… On 18 Aug 2020, at 11:59, Nate Wright ***@***.***> wrote:
@NateWr requested changes on this pull request.
Thanks @gemusehandler <https://github.com/gemusehandler>! Nice work. I've added some comments to the code to reflect some minor syntax and whitespace changes in order to fit our coding style.
I've also provided a commit that just re-arranges your code a little bit so that the series index can be found a /catalog/series instead of /catalog/seriesIndex. You can see that at gemusehandler#1 <gemusehandler#1> and you should be able to just merge it into your code.
In templates/frontend/pages/catalogSeriesIndex.tpl <#855 (comment)>:
> @@ -0,0 +1,66 @@
+{**
+ * templates/frontend/pages/catalogSeries.tpl
+ *
+ * Copyright (c) 2014-2017 Simon Fraser University Library
+ * Copyright (c) 2003-2017 John Willinsky
+ * Distributed under the GNU GPL v2. For full terms see the file docs/COPYING.
+ *
+ * @brief Display the page to view books in a series in the catalog.
+ *
+ * @uses $series Series Current series being viewed
+ * @uses $publishedMonographs array List of published monographs in this series
+ * @uses $featuredMonographIds array List of featured monograph IDs in this series
+ * @uses $newReleasesMonographs array List of new monographs in this series
+ *}
+{include file="frontend/components/header.tpl" pageTitle="plugins.block.browse.series"}
Replace plugins.block.browse.series with series.series here and everywhere else that it is used.
In templates/frontend/pages/catalogSeriesIndex.tpl <#855 (comment)>:
> @@ -0,0 +1,66 @@
+{**
+ * templates/frontend/pages/catalogSeries.tpl
This should be catalogSeriesIndex.tpl
In templates/frontend/pages/catalogSeriesIndex.tpl <#855 (comment)>:
> @@ -0,0 +1,66 @@
+{**
+ * templates/frontend/pages/catalogSeries.tpl
+ *
+ * Copyright (c) 2014-2017 Simon Fraser University Library
+ * Copyright (c) 2003-2017 John Willinsky
+ * Distributed under the GNU GPL v2. For full terms see the file docs/COPYING.
+ *
+ * @brief Display the page to view books in a series in the catalog.
+ *
+ * @uses $series Series Current series being viewed
+ * @uses $publishedMonographs array List of published monographs in this series
+ * @uses $featuredMonographIds array List of featured monograph IDs in this series
+ * @uses $newReleasesMonographs array List of new monographs in this series
The @brief and @uses declarations need to be updated to reflect this template.
In templates/frontend/pages/catalogSeriesIndex.tpl <#855 (comment)>:
> @@ -0,0 +1,66 @@
+{**
+ * templates/frontend/pages/catalogSeries.tpl
+ *
+ * Copyright (c) 2014-2017 Simon Fraser University Library
+ * Copyright (c) 2003-2017 John Willinsky
Update the copyright to go to 2020.
In templates/frontend/pages/catalogSeriesIndex.tpl <#855 (comment)>:
> + * @uses $featuredMonographIds array List of featured monograph IDs in this series
+ * @uses $newReleasesMonographs array List of new monographs in this series
+ *}
+{include file="frontend/components/header.tpl" pageTitle="plugins.block.browse.series"}
+
+<div class="page page_catalog_seriesIndex">
+
+ {* Breadcrumb *}
+ {include file="frontend/components/breadcrumbs_catalog.tpl" type="series" currentTitleKey="plugins.block.browse.series"}
+
+ <h2>
+ {translate key="plugins.block.browse.series"}
+ </h2>
+
+ {* Index with series *}
+ <div class="seriesIndex">
Let's call this class series_list to follow the naming conventions of other classes, like cmp_monograph_list.
In templates/frontend/pages/catalogSeriesIndex.tpl <#855 (comment)>:
> +
+ {* Breadcrumb *}
+ {include file="frontend/components/breadcrumbs_catalog.tpl" type="series" currentTitleKey="plugins.block.browse.series"}
+
+ <h2>
+ {translate key="plugins.block.browse.series"}
+ </h2>
+
+ {* Index with series *}
+ <div class="seriesIndex">
+ <ul>
+
+ {* Series *}
+ {if $browseSeriesFactory && $browseSeriesFactory->getCount()}
+
+ {iterate from=browseSeriesFactory item=browseSeriesItem}
Double-check the indentation here and throughout the rest of this template. HTML elements and smarty tags should be indented, like:
<ul>
{if ...}
{iterate ...}
<li>...</li>
{/iterate}
{/if}
</ul>
In templates/frontend/pages/catalogSeriesIndex.tpl <#855 (comment)>:
> + {* Index with series *}
+ <div class="seriesIndex">
+ <ul>
+
+ {* Series *}
+ {if $browseSeriesFactory && $browseSeriesFactory->getCount()}
+
+ {iterate from=browseSeriesFactory item=browseSeriesItem}
+ <li>
+
+ <div class="imageDescription">
+
+ {* Image and description *}
+
+ <div class="cover"> <a href="{url router=$smarty.const.ROUTE_PAGE page="catalog" op="series" path=$browseSeriesItem->getPath()|escape}">
+ <img src="{url router=$smarty.const.ROUTE_PAGE page="catalog" op="thumbnail" type="series" id=$browseSeriesItem->getId()|escape}" alt="{$browseSeriesItem->getLocalizedFullTitle()|escape}" /></a>
Let's only show an image if it exists. You can do this:
{assign var="image" value=$browseSeriesItem->getImage()}
{if $image}
<img ...>
{/if}
In templates/frontend/pages/catalogSeriesIndex.tpl <#855 (comment)>:
> + {* Series *}
+ {if $browseSeriesFactory && $browseSeriesFactory->getCount()}
+
+ {iterate from=browseSeriesFactory item=browseSeriesItem}
+ <li>
+
+ <div class="imageDescription">
+
+ {* Image and description *}
+
+ <div class="cover"> <a href="{url router=$smarty.const.ROUTE_PAGE page="catalog" op="series" path=$browseSeriesItem->getPath()|escape}">
+ <img src="{url router=$smarty.const.ROUTE_PAGE page="catalog" op="thumbnail" type="series" id=$browseSeriesItem->getId()|escape}" alt="{$browseSeriesItem->getLocalizedFullTitle()|escape}" /></a>
+ </div>
+
+ <div class="metadata">
+ <h3><a href="{url router=$smarty.const.ROUTE_PAGE page="catalog" op="series" path=$browseSeriesItem->getPath()|escape}"> {$browseSeriesItem->getLocalizedFullTitle()|escape}</a></h3>
Change h3 to h2 to reflect the page structure.
In templates/frontend/pages/catalogSeriesIndex.tpl <#855 (comment)>:
> +
+ {iterate from=browseSeriesFactory item=browseSeriesItem}
+ <li>
+
+ <div class="imageDescription">
+
+ {* Image and description *}
+
+ <div class="cover"> <a href="{url router=$smarty.const.ROUTE_PAGE page="catalog" op="series" path=$browseSeriesItem->getPath()|escape}">
+ <img src="{url router=$smarty.const.ROUTE_PAGE page="catalog" op="thumbnail" type="series" id=$browseSeriesItem->getId()|escape}" alt="{$browseSeriesItem->getLocalizedFullTitle()|escape}" /></a>
+ </div>
+
+ <div class="metadata">
+ <h3><a href="{url router=$smarty.const.ROUTE_PAGE page="catalog" op="series" path=$browseSeriesItem->getPath()|escape}"> {$browseSeriesItem->getLocalizedFullTitle()|escape}</a></h3>
+
+ <div class="description">{$browseSeriesItem->getLocalizedDescription()|strip_unsafe_html|truncate:800}</div>
Let's not truncate this string. It will cut it off mid-word. Instead, we prefer to let each journal decide the length that is appropriate by adjusting the description.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#855 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABDGQSZEOEAS34PSKSKLDQLSBJGGJANCNFSM4QAMXUZA>.
|
Hi @gemusehandler, were you able to make the changes? It doesn't look like there are any new commits in this pull request, so you might need to push them up still. |
Ha Nate, sorry it took some time. Start of the new academic year is always a busy time.
I have gone through all the comments and made changes accordingly in the
templates/frontend/pages/catalogSeriesIndex.tpl <https://github.com/pkp/omp/pull/855/files#diff-463610f59a1419f56b1d3fefc4210518>
Please let me know if I still need to do anything
Still working on my Github skills.
Frank (aka gemusehandler)
… On 9 Sep 2020, at 10:26, Nate Wright ***@***.***> wrote:
Hi @gemusehandler <https://github.com/gemusehandler>, were you able to make the changes? It doesn't look like there are any new commits in this pull request, so you might need to push them up still.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#855 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABDGQS6GSZO43XAN5YNSMT3SE432VANCNFSM4QAMXUZA>.
|
No description provided.