Skip to content

Book details page - #57

Open
YegorUdovchenko wants to merge 115 commits into
masterfrom
book-details-page
Open

Book details page#57
YegorUdovchenko wants to merge 115 commits into
masterfrom
book-details-page

Conversation

@YegorUdovchenko

@YegorUdovchenkoYegorUdovchenko commented Apr 16, 2018

Copy link
Copy Markdown
Contributor

In this PR the book details page view was implemented. It is shown after the book tile click or passing the following URL directly: /#/book-details/{book-id}/{book-title}.
The {book-title} part is not functional. It is used to add meaning to the URL.

The following custom elements were created and tested:

  • <exlibris-book-details> - the main details view component. It applies the ExlibrisApp.ReduxMixin. It is used to dispatch the book related actions provided by this view and to receive the book from the application state.
    This component displays whole book description with the cover. Also, it displays the status messages and the button. Those values are calculated from the book status and other specific book properties.
    The book loading and notFound fields were added to indicate the book loading failure or the loading delay. It allows showing view 404 on invalid Id values passed and the parer-spinner when the loading delay happen. To implement this functionality, some temporary functions were added to imitate server response with a delay.

  • <exlibris-book-details-redirect-link> the part of book details component used to create the external link to the specified book. The link generation depends on the book code format.

The book details page examples:

1
2
3

The same page on the mobile screen:
7
6
8

# Conflicts:
#	client-web/src/exlibris-app/exlibris-app.html
# Conflicts:
#	client-web/src/exlibris-app/exlibris-app.html
# Conflicts:
#	client-web/src/exlibris-app/exlibris-app.html
# Conflicts:
#	client-web/src/exlibris-app/exlibris-app.html
# Conflicts:
#	client-web/src/exlibris-app/exlibris-app.html
@YegorUdovchenko

Copy link
Copy Markdown
ContributorAuthor

@dkalinin PTAL

@dkalinindkalinin left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

return Object.assign({}, state, {
bookDetails: {
book: {},
loadingStatus: ExlibrisApp.LoadingStatus.NOT_FOUND

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Empty book + loaded:false = Book not found.
Book + loaded:false = Completed.
It's enough to have boolean property loaded instead of enum LoadingStatus.

if (Array.isArray(arguments[i])) {
const filteredArray = arguments[i].filter(x => x.id === id);
if (filteredArray.length > 0) {
element = filteredArray[0];

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

booksAndButtonsCombinations.forEach(function (combination) {
const element = fixture('BasicTestFixture');
element.set('book', combination.book);
testElements.push(element);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It's better to create a separate test for each combination:

suite(...,()=>{booksAndButtonsCombinations.forEach(function(combination){test(`should display book action button with title ${combination.book}`,done=>{constelement=fixture('BasicTestFixture');element.set('book',combination.book);constwideScreenButton= ....});});})

~ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
-->

<link rel="import" href="../shell/exlibris-app-namespace.html">

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Project imports should be after bower imports.

<link rel="import" href="../shell/exlibris-app-namespace.html">
<link rel="import" href="../../../bower_components/polymer/polymer-element.html">
<link rel="import" href="../../../bower_components/polymer-redux/polymer-redux.html">
<link rel="import" href="../../../bower_components/polymer/lib/elements/dom-if.html">

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It's not used.


<link rel="import" href="../shell/exlibris-app-namespace.html">
<link rel="import" href="../../../bower_components/polymer/polymer-element.html">
<link rel="import" href="../../../bower_components/polymer-redux/polymer-redux.html">

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It's imported in redux-store.

* @property {boolean} isAllowedLoanExtension true if the loan extension is provided.
* @property {Date} date the date when the book is expected to become available.
* @property {Date} dueDate the date when the book should be returned.
* @property {BookStatus} status one of the BookStatus values.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

If BookStatus is type it should be described in @typedef before.

* @returns {Object} the title and the click function of the button for the specified book.
* @private
*/
_chooseButton(book) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It's enough to have status and isAllowedLoanExtension params.

* for the specified book.
* @private
*/
_calculateActionUnsupportedMessage(book) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It's enough to have status and isAllowedLoanExtension params.

@dkalinin

Copy link
Copy Markdown

@YegorUdovchenko Gray on gray is not readable:
image

@YegorUdovchenkoYegorUdovchenko added the wip Work in progress label Jun 7, 2018
@YegorUdovchenko

Copy link
Copy Markdown
ContributorAuthor

@YuriiHaidamaka PTAL

@YuriiHaidamakaYuriiHaidamaka left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, but fix comments, please.

// Searching for a book in a local data.
// Should be removed when implement server response
const id = action.bookId;
const book = findElement(id, state.allBooks, state.expectedSoonBooks, state.borrowedBooks, state.reservedBooks);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it will be better here.

 const book = [...state.allBooks, ...state.expectedSoonBooks, ...state.borrowedBooks, ...state.reservedBooks].find((item) => item.id === id);

* The book details page component.
*
* This component allows to display the book details page with the book
* specific messages, the book cover and the button when it is supported.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please delete redundant space.

}
};

function findElement(id) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If you did the above change you would be able to delete this function.

@YegorUdovchenkoYegorUdovchenko removed the wip Work in progress label Jun 18, 2018
@YegorUdovchenko

Copy link
Copy Markdown
ContributorAuthor

@dkalinin PTAL

@YuriiHaidamakaYuriiHaidamaka left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

Sign up for freeto 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.

3 participants

@YegorUdovchenko@dkalinin@YuriiHaidamaka