Opened 5 years ago

Last modified 4 years ago

#12918 review_failed Task

Kama in Retina

Reported by: Chris Graham Owned by: Olek Nowodziński
Priority: Normal Milestone:
Component: UI : Skins Version:
Keywords: Cc:

Description

If we contribute high quality 2x versions of the assets you have in git, would you guys take these on?

Attachments (4)

12918-diff.gif (95.6 KB) - added by Olek Nowodziński 5 years ago.
Differences between Lo- and HiDPI icons.
kama1.png (79.6 KB) - added by Piotrek Koszuliński 4 years ago.
kama2.png (105.3 KB) - added by Piotrek Koszuliński 4 years ago.
kama3.png (39.6 KB) - added by Piotrek Koszuliński 4 years ago.

Download all attachments as: .zip

Change History (17)

comment:1 Changed 5 years ago by Wiktor Walc

Definitely! Feel free to make a pull request, I'm sure lots of Kama users will be happy seeing such improvement.

comment:2 Changed 5 years ago by Jakub Ś

Status: newconfirmed

comment:3 Changed 5 years ago by Chris Graham

comment:4 Changed 5 years ago by Piotrek Koszuliński

Milestone: CKEditor 4.5.0

comment:5 Changed 5 years ago by Olek Nowodziński

Owner: set to Olek Nowodziński
Status: confirmedassigned

comment:6 Changed 5 years ago by Olek Nowodziński

Differences between Lo- and HiDPI icons.

Last edited 5 years ago by Olek Nowodziński (previous) (diff)

Changed 5 years ago by Olek Nowodziński

Attachment: 12918-diff.gif added

Differences between Lo- and HiDPI icons.

comment:7 Changed 5 years ago by Olek Nowodziński

Milestone: CKEditor 4.5.0
Resolution: invalid
Status: assignedclosed

See my comment in the original thread on GitHub.

comment:8 Changed 5 years ago by Olek Nowodziński

Milestone: CKEditor 4.5.0
Resolution: invalid
Status: closedreopened

comment:9 Changed 5 years ago by Piotrek Koszuliński

Milestone: CKEditor 4.5.0
Status: reopenedconfirmed

comment:10 Changed 4 years ago by Olek Nowodziński

Status: confirmedreview

I pushed several new commits to t/12918.

comment:11 Changed 4 years ago by Piotrek Koszuliński

Status: reviewreview_failed

I rebased and pushed a commit with a manual test to the branch:t/12918.

I found couple of issues:

  1. kama/images/hidpi/mini.gif does not exist. A mini.png file does. A 404 error is thrown when opening any dialog. (See kama1.png)
  2. The "x" on the cancel button in the dialogs looks differently than before. It has a different color. (See kama1.png)
  3. When I built a package using kama some of the icons got a black backgrounds (2 in the forms plugin and the table). (See kama2.png)
  4. The Bold button is one of the most important ones and still differs much from the lowdpi one.
  5. Icons are not aligned vertically. If you compare kama3.pnd and kama2.png you should notice that the old icons like sub and sup or new page and preview or table and hr or numbered list and bulleted list or link and unlink. They were all well aligned. Now there's no order.
  6. There's something wrong with the bidi icons. (See kama2.png)
  7. The cut icon do would need some tweaking too.

While most of these issues does not require a lot of work I'm worried by two things:

  • Number of them (and I really wasn't that picky).
  • The general feeling. The low dpi icons may be blurred on Retina, but they are consistent, well aligned, lines have the same thickness and they are pretty clean. The new ones have problems in all these aspects so when I look at the whole editor something is not right.

Therefore, before we spend more time on this I want to ask if it's worth the effort. I know that a lot of work was already done (on your side Chris, but also on our), but I'm afraid that we are still far. The current state (no hidpi icons) is not that bad so I will have problems with accepting a solution which could be seen as a regression by some people.

I don't want to force this view, but my proposal is that we can fix the issues 1. and 3. and 5. and leave this branch. If anyone wants hidpi icons they can use it, so the work is not wasted. But we will not merge it to the core to avoid spending more time on the rest of issues and lowering quality of the skin.

Last edited 4 years ago by Piotrek Koszuliński (previous) (diff)

Changed 4 years ago by Piotrek Koszuliński

Attachment: kama1.png added

Changed 4 years ago by Piotrek Koszuliński

Attachment: kama2.png added

Changed 4 years ago by Piotrek Koszuliński

Attachment: kama3.png added

comment:12 Changed 4 years ago by Chris Graham

My point of view is that on an iMac, looking at the editor within a site that uses retina graphics, it looks very dated and out of place. While using moono, it's only really appropriate on some site designs, otherwise that looks out of place also.

So I do think lack of retina graphics is a serious issue that ages CKEditor, and hence why we invested around 2 weeks of design time into resolving it.

There are 86 icons, so that's only 1.5 hours per icon for the 2 week figure. It's not that surprising that there'd be some issues while churning through that quantity of work. We didn't use zooming in as a part of our review process on our end, or do direct comparisons between low-dpi and high-dpi, and we didn't go through your build process as it's not easy for us - but now we know these issues, I see no reason why we can't resolve them. We also weren't thinking about line thickness, as on low-dpi that is merely a function of the pixel size -- but we can't take that into account also.

If you're not going to merge, we'll have to give up, because we're not going to want to maintain a fork. But I think it is an important issue and I don't want to give up on it. We've waited such a long time between each review, and it would just be so disappointing to have to drop it all.

comment:13 Changed 4 years ago by Piotrek Koszuliński

As I wrote, I don't want to force my view and stop you :). If you're willing to work on it, we're keep reviewing and helping.

As for maintaining this branch if it wouldn't be merge – I think it would be easy. There will not be many changes in this code in the future... If any.

Note: See TracTickets for help on using tickets.
© 2003 – 2019 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy