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

Make edition component responsive #1899

Conversation

koderjoker
Copy link
Contributor

Description: What does this PR achieve? [hotfix]

Makes the edition component responsive

Closes #1339

Evidence: If this PR touches UI, please post evidence (screenshot) of it behaving correctly:

For laptops screens

For mobile

@koderjoker
Copy link
Contributor Author

koderjoker commented Feb 17, 2019

Changes required:

  • Change the use of editions id to follow linting rules
  • Make sure the interface doesn't break at tablet breakpoint (768px) while maintaining mobile first approach

The interface breaks exactly at 768px. Most likely because of overlapping styles of editions.less (uses min-width) and common.less (uses max-width).

@tfmorris
Copy link
Contributor

Is there a way to reduce the copious amounts of whitespace in both the desktop and mobile layouts? Doesn't the current desktop put the title, etc, next to the cover image? Because of the aspect ratio of most covers (tall, narrow), there's a lot of space wasted if nothing is put next to them.

For the mobile version, perhaps the "Read" button could be moved up there. We certainly don't need a full width Read button.

I'd also consider using different data elements on desktop and mobile. For example, is the publisher really important enough to take up valuable mobile screen real estate?

@koderjoker koderjoker changed the title Make edition component responsive WIP: Make edition component responsive Feb 19, 2019
@koderjoker
Copy link
Contributor Author

@cdrini @tfmorris I'll create some UI prototypes with your respective suggestions to show how they look!

@koderjoker
Copy link
Contributor Author

Samples:

Reducing whitespace by shifting text to the right
shift text

Shifting download links to a dropdown (to overcome problem of whitespace and make download links bigger for mobile)
download button

@cdrini
Copy link
Collaborator

cdrini commented Feb 27, 2019

That looks muuuch nicer! I think the download button is interesting, but in the interest of progress, let's drop it. I think it would take too long to make it look nice/work smoothly. Could you show a mockup with multiple books? Also place the DAISY link below the read button (so it's consistent with the desktop version of the site).

@mekarpeles
Copy link
Member

agree, the download button as designed is problematic for a variety of reasons including that the reading log button is green (conflicting color scheme)

I'd just add it as an option either under the read button, or as another option besides the other download formats.

@koderjoker
Copy link
Contributor Author

@mek Actually, I wasn't sure what component to use for a dropdown so I used the reading long button just to provide a rough sample of how a dropdown download button would look. 😆 Didn't intend for it to be used in the long run.

I'm willing to design and implement a download button next if it is wanted.

@cdrini @jdlrobson Mockup with multiple books on mobile
screen shot 2019-02-28 at 2 48 10 pm

@cdrini
Copy link
Collaborator

cdrini commented Feb 28, 2019

Awesome! That's coming along nicely. If we want a download button, I think we should investigate it in a new issue; for now let's not worry about it. Ok, some final changes:
There is still a little too much whitespace. This section's primary function is to a list of data, so we want it tight.

  • Could you remove pretty much any vertical space? (Above the read button, below the download link, between edition entries).
  • Could we have "worldcat" on the same line as "libraries near you?"
  • would it be possible to have the download links on the same line as print disabled?
  • increase the font size of the small text. Especially download/library links so that they are easier to hit on mobile.
  • remove that line under print disabled; we're also using lines to separate the edition entries, so it's a little confusing

Thanks for your patience, @koderjoker ! Assuming nothing goes horribly wrong, this should be the last set of changes :P

@koderjoker
Copy link
Contributor Author

Firstly just summarising the structure of the table that is being reformatted, to make referring back to some stuff easier

For every tr
td1- Contains the book cover and title data
td2- Contains the read book button, daisy link and download links
td3- Contains library links
td4- Contains links for purchasing

With multiple books:
screen shot 2019-03-01 at 10 27 18 pm

Close up for mobile:
screen shot 2019-03-01 at 10 31 04 pm

For mobile:

  • Adjusted the padding as much as I could! Any other extra space is due to the absence of table data in the table row.
  • "Libraries near you" could have multiple links associated with it (WorldCat, Library.link etc), so I think it would be best to have the links on the next line, to avoid the links wrapping to the next line
  • Same reason as above, unless the daisy URL is shortened
  • Increased the font size to make easier to hit the links!
  • Removed the separator under the daisy link!

For tablet
screen shot 2019-03-01 at 10 31 58 pm

Unfortunately, the tablet version still needs changes :p

For starters, the width of the 'read eBook button' definitely needs to be reduced. We then could have the 'daisy and download links' on one line and the 'library and purchasing links' on another, but the read ebook button would still have space left over on the side...

We could overcome that with a future download button though. The 'read ebook and download button' on one line, and 'library links and purchase links' on another.

@@ -30,7 +30,6 @@
position: relative;
z-index: @z-index-level-1;
margin: 15px 0;
float: left;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are you sure this doesn't break any other dataTables (I don't know the answer)?
If you're not confident, I'd suggest keeping this and adding a more specific CSS selector inside static/css/components/editions.less to override this rule.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jdlrobson this change is no longer necessary in the latest changes I made, so I'll leave it unchanged in the next commit 👍

@jdlrobson jdlrobson changed the title WIP: Make edition component responsive Make edition component responsive Mar 1, 2019
@jdlrobson
Copy link
Collaborator

This looks really good to me. I would be open to merging this as is provided we iterate on top of it. With that in mind I've removed the WIP tag ( please see #1339 (comment))

Great work @koderjoker it's great to start seeing these mobile experiences come to life!

@koderjoker koderjoker closed this Mar 2, 2019
@koderjoker koderjoker force-pushed the 1339/hotfix/improve-editions-component-interface branch from c106d9b to cf19feb Compare March 2, 2019 19:15
@koderjoker koderjoker reopened this Mar 2, 2019
@koderjoker
Copy link
Contributor Author

@jdlrobson @mekarpeles @cdrini @tfmorris

Final look for

Mobile
screen shot 2019-03-03 at 12 32 12 am

Tablet
screen shot 2019-03-03 at 1 02 38 am

@cdrini
Copy link
Collaborator

cdrini commented Mar 3, 2019

Nice @koderjoker! I know this has dragged on for a bit, but I think the design of this has improved greatly :) Here are some final notes/comments:

  • Could you test this with a borrowable book that's been borrowed? Usually that would cause 2 buttons to appear: "Read" and "Return"; do they still display ok?
  • Hmm, maybe try adding commas to separate the libraries (you can use CSS pseudo-elements to do this)? I think keeping them on one line is still better, even if there's a risk they'll wrap sometimes. An interface that is good 95% of the time and ok 5% of the time is better than an interface that is ok 100% of the time.
  • Would it be possible to hide empty cells using the CSS :empty pseudo-selector?
  • agreed the tablet view looks a little funky :P in the interest of progress, I'd suggest opening a new issue for that once this is merged.

@mekarpeles
Copy link
Member

mekarpeles commented Mar 3, 2019 via email

@koderjoker
Copy link
Contributor Author

For every tr
td1- Contains the book cover and title data
td2- Contains the read book button, daisy link and download links
td3- Contains library links
td4- Contains links for purchasing

@mekarpeles do you mean that I should shift the 'Read eBook button' to the bottom of it's td and tr?

@mekarpeles
Copy link
Member

mekarpeles commented Mar 4, 2019 via email

@cdrini
Copy link
Collaborator

cdrini commented Mar 4, 2019

(FYI, if the tr element is a flex box container, you can change the order of the cells using just css)

@koderjoker
Copy link
Contributor Author

Yup @cdrini, I'm aware of the above. Will implement the new changes as specified.

@koderjoker
Copy link
Contributor Author

@mekarpeles @cdrini @jdlrobson as per changes suggested

Single book sample
screen shot 2019-03-06 at 12 27 22 am

With multiple books
screen shot 2019-03-06 at 12 37 41 am

@cdrini
Copy link
Collaborator

cdrini commented Mar 5, 2019

Sweet! Lgtm. Can we get this on dev, @mekarpeles ?

@mekarpeles
Copy link
Member

@cdrini + @koderjoker this is live on https://dev.openlibrary.org, let's test!

@cdrini
Copy link
Collaborator

cdrini commented Mar 6, 2019

Oh, @koderjoker I don't think you've pushed your latest changes :P the thing on dev is what you had a few days ago.

@koderjoker
Copy link
Contributor Author

Wait, this wasn't supposed to happen 😂 I had put up the UI screenshots up first so that they could be approved before I pushed the latest changes!
I'll push the changes once I get access to a computer!

@koderjoker
Copy link
Contributor Author

koderjoker commented Mar 6, 2019

@mekarpeles @jdlrobson @cdrini encountering a couple of errors with the css build size

static/build/page-admin.css: 19.06KB > maxSize 19KB (gzip)
static/build/page-form.css: 18.76KB > maxSize 18.7KB (gzip) 
static/build/page-user.css: 18.62KB > maxSize 18.6KB (gzip)

@jdlrobson
Copy link
Collaborator

@mekarpeles @jdlrobson @cdrini encountering a couple of errors with the css build size

static/build/page-admin.css: 19.06KB > maxSize 19KB (gzip)
static/build/page-form.css: 18.76KB > maxSize 18.7KB (gzip) 
static/build/page-user.css: 18.62KB > maxSize 18.6KB (gzip)

These are all minor, so you should bump to them (but acknowledge the increases) in the commit message. You can find these values defined in package.json. While these values ideally would be going down, I think in this situation the increase is necessary, so you have my blessing to increase them :)

Maximum bundle size increased for
page-admin, page-form, page-plain and page-user.
@koderjoker
Copy link
Contributor Author

koderjoker commented Mar 7, 2019

UI issue for pages with multiple editions!

screen shot 2019-03-07 at 11 39 42 pm

Suggested changes:

screen shot 2019-03-08 at 12 13 57 am

  • Remove all extraneous floats used in the page (and then check to make sure it doesn't break other data tables)
  • Shift number buttons below first, previous, next and last buttons for mobile devices
  • Increase margin for certain elements

@jdlrobson
Copy link
Collaborator

Love the suggested fix!!

@mekarpeles
Copy link
Member

(copied from my msg in slack: https://internetarchive.slack.com/archives/C0ETZV72L/p1551996974120600?thread_ts=1551902952.094300&cid=C0ETZV72L)

The <tr> with border-bottom: #999 should probably be a lighter grey and also have a bit more padding-bottom.

Currently the “DAISY for print-disabled” is very cramped. It looks like #editions tr .print-disabled-download requires ~10px padding-bottom

re: border-bottom, perhaps something closer to hsla 0, 0, 87% (I think we have a light-grey in static/css/less/colors.less)

before

screen shot 2019-03-07 at 14 19 34pmpst

after

screen shot 2019-03-07 at 14 19 00pmpst

@koderjoker
Copy link
Contributor Author

koderjoker commented Mar 8, 2019

Using flex order has affected the tab order. Got to resolve that!

tab order

Another thing I noticed while tabbing through the page with voice over is that a user isn't informed when an eBook isn't available, and the library links' placement isn't clear on the mobile and tablet version of the component. Could work on those features too, or add them in another PR.

@mekarpeles
Copy link
Member

mekarpeles commented Mar 8, 2019

This is really coming along, great work @koderjoker!

Is there a checklist of remaining todos? Or is this ready for review

(I think tabbing is reasonable to make a separate issue -- I've created it here: #1963)

@koderjoker
Copy link
Contributor Author

Ready for review!

@tfmorris
Copy link
Contributor

Did reducing with width of the "Read" button get lost? I commented on it a few weeks ago and @koderjoker said:

For starters, the width of the 'read eBook button' definitely needs to be reduced. We then could have the 'daisy and download links' on one line and the 'library and purchasing links' on another, but the read ebook button would still have space left over on the side...

which sounded like agreement that it was too wide. Paradoxically, it's fine when the screen is wide, but below ~770 pix, it jumps to full width.

Also, a nit, but the Library.link styling looks weird/confusing. In the mobile rendering it looks like it's a modifier to WorldCat rather than a separate thing. Part this is because the label isn't capitalized correctly (ie. Library.Link).

Probably not for this ticket, but I wonder why we're sending any traffic to a commercial service which has a tiny fraction of the holdings of WorldCat. As long as we're keeping it though, we should at least make it clear to the users where we're sending them off to.

@mekarpeles
Copy link
Member

@tfmorris -- Library.Link is a team that spun out of oclc and they've been partnering directly with Internet Archive. Agreed re: presentation that opening a ticket would be great

re: full sized button, I also think this a good point -- may I also open this as a followup issue so this can finally be merged? 🎉

@mekarpeles mekarpeles merged commit 6c9df3a into internetarchive:master Mar 17, 2019
@mekarpeles
Copy link
Member

congrats @koderjoker !

@koderjoker koderjoker deleted the 1339/hotfix/improve-editions-component-interface branch April 22, 2019 02:49
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.

Editions component on mobile
5 participants