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

Refactor/colorbox design #2195

Merged
merged 7 commits into from
Jun 22, 2019
Merged

Conversation

tabshaikh
Copy link
Collaborator

Description

Subtask of #659

Technical

The OL colorbox modal has been resized and redesigned

Evidence

Screenshot from 2019-06-20 18-50-58
Screenshot from 2019-06-20 18-50-41

@tabshaikh
Copy link
Collaborator Author

The next steps to refactor the modal will be to change the close arrow to something smaller and its color to black/red as it suits the modal

@tabshaikh tabshaikh requested a review from mekarpeles June 20, 2019 13:22
@mekarpeles
Copy link
Member

This lgtm so far, let's fix the X before merging into master

@mekarpeles
Copy link
Member

@koderjoker may be able to propose advice on the best way to replace the clunky "X" graphic in our modal.

Right now the modal uses a.floaterShut { background-image: url(/images/icons/icon_close-pop.png); }

Which is implemented here:
<a class="floaterShut" href="javascript:;" onclick="$.fn.colorbox.close();"><span class="shift">Close</span></a>

We may want to instead use:
https://fontawesome.com/v4.7.0/icon/times

Or just a simple x (to avoid loading any new font dependencies)

Thoughts?

@koderjoker
Copy link
Contributor

I agree that it would be best to avoid loading any new font dependencies.

How about using the unicode for multiplication &times; × as a symbol, since it's centred and symmetric, and include it as a button with an aria-label?
<button aria-label="Close Modal">&times;</button>

@mekarpeles
Copy link
Member

mekarpeles commented Jun 21, 2019

Yeah, I like @koderjoker's solution, here's what I think we should do:
Screen Shot 2019-06-21 at 12 29 19PMPDT

  1. set floaterHead to display: flex so we don't need floats
  2. set div.floaterHead h2 to flex: 1 so it takes up all the space (other than the ×)
  3. remove background-image, position: absolute, and top: 0 and right: 0 from a.floaterShut
  4. change the contents of <a class="floaterShut" href="javascript:;" onclick="$.fn.colorbox.close();">...</a> to be <a class="floaterShut" href="javascript:;" onclick="$.fn.colorbox.close();">×</a>
  5. add text-decoration: none; on a.floaterShut and make font-size: 1.1em; and color: @grey or some sort of grey which is close to #666

@tabshaikh
Copy link
Collaborator Author

The last commit probably needs more testing from your side @mekarpeles :)

@mekarpeles
Copy link
Member

:(

  1. a grep -rli "icon_close" reveals that icon_close-pop.pngis still referenced inui-dialog.less`.
  2. Edit Book Covers, click "Manage". The parent container is not large enough

Let's see if/where #1 is used and come up with a solution to #2.

@mekarpeles
Copy link
Member

The size/clipping issue was related to the declared size of the iframe. I've fixed this and will push

@mekarpeles mekarpeles force-pushed the refactor/colorbox-design branch from b3a199e to 0f62796 Compare June 22, 2019 20:13
@mekarpeles
Copy link
Member

Here's an example of where things break! :) No ×
Screen Shot 2019-06-22 at 13 15 34PMPDT

@mekarpeles mekarpeles force-pushed the refactor/colorbox-design branch 2 times, most recently from d831841 to 6ddb946 Compare June 22, 2019 20:19
@mekarpeles
Copy link
Member

There are the files affected by the &times; change. We should go down the list and make sure each works correctly before merging:

templates/covers/change.html
templates/covers/author_photo.html
templates/covers/book_cover_single_edition.html
templates/covers/book_cover.html
templates/covers/book_cover_work.html
templates/type/edition/view.html
templates/lists/widget.html
templates/books/edit/addcover.html
templates/books/edit/addfield.html
templates/lib/markdown.html
macros/databarAuthor.html

@mekarpeles mekarpeles force-pushed the refactor/colorbox-design branch from 6ddb946 to 6e056d8 Compare June 22, 2019 20:43
- not rendering mobile test for iframes in forms
- fixes modal inner iframe sizing (book covers)
- consistent styling for colorbox and ui-tabs
- fixes author cover variants of modal as well
@mekarpeles mekarpeles force-pushed the refactor/colorbox-design branch from 6e056d8 to e882105 Compare June 22, 2019 21:21
@mekarpeles
Copy link
Member

All issues seem resolved + paths working.

@tabshaikh
Copy link
Collaborator Author

Great then let's merge that

@mekarpeles mekarpeles merged commit 3f73bd2 into master-sponsorship Jun 22, 2019
@mekarpeles mekarpeles deleted the refactor/colorbox-design branch June 22, 2019 21:53
@mekarpeles
Copy link
Member

Merged into master as well.

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.

3 participants