WebMemex / webmemex-extension

📇 Your digital memory extension, as a browser extension
https://webmemex.org
Other
208 stars 45 forks source link

Issue #40 -- Overhaul overview (search results) #55

Closed reficul31 closed 7 years ago

reficul31 commented 7 years ago

This is in reference to the issue. screenshot from 2017-03-05 20 16 29

Treora commented 7 years ago

I just tried it out, this design looks more informative than the current squares. A bunch of small things I would change still (some were already discussed in #40):

And possibly some other small things I don't notice now.

reficul31 commented 7 years ago

Thanks for the review. I'll change right now.

reficul31 commented 7 years ago

Updated PR:

  1. Added significant spacing between the buttons.
  2. Removed the clustering from the ResultView
  3. Changed text-overflow to ellipsis
  4. "No Screenshot Available" made thinner and more lighter
  5. Background image back up

Please review and tell me what else should be changed. Thanks for your time.

Treora commented 7 years ago

Nice. Sorry to keep commenting, but I just played around with it more, finding that the use of relative sizes everywhere has its problems. E.g. a small window size just looks very wrong:

relative-sizes-problem

The layout seems to be designed to be nice on a normal screen size, but that is not the right way to design things. I see you also use display:table for vertical alignment, and float left and right , and wondered if you know that nowadays there css has flex layout. It takes a moment to get into it though, but allows making such things nicer and more.. flexible. I can also have a try at improving it some moment, or maybe somebody with more design skills than I can.

reficul31 commented 7 years ago

I am so sorry I forgot the min-height of the component(rookie mistake). I will try and be more careful next time. Obviously that doesn't solve the responsive problem. I have changed the PR. I looked into flex and it seems it solves some of our responsive problem. I will change the code again and make the PR. Thanks a lot for pointing out the mistakes. Any opportunity to learn is a good one. :D

Chaitya62 commented 7 years ago

Just a suggestion but why don't we add a placeholder icon when we cannot get the fa Vicon instead of that white page icon.

obsidianart commented 7 years ago

I see a large use of percentages for sizing. Those are cards and they don't really make sense in percentage sizing for height. Percentage is a very tricky thing to use, the value depends on the first parent specified in pixel for width and height but on the element for padding (and probably margin), and you can't move the card from one place to another without risking to break it. I don't think they percentages are needed to style those cards. About the 3 columns system, I would probably evaluate flexbox rather than floating, since this is for modern browser, but this can open more issues and it can be done later.

reficul31 commented 7 years ago

Thank you @obsidianart for the review. Yeah as pointed by @Treora I was going to switch over from percentages and move to more definite sizing scheme. Currently I am in the process of converting all the floats to flex. The PR shall be updated soon. Sorry for the css practises.

reficul31 commented 7 years ago

Added the initial implementation of the above designed front end with flex box css. I have tried to conform to the styles mentioned in the previous PR that I made. @obsidianart @Treora please review the PR and tell me all the changes to be made.

obsidianart commented 7 years ago

I still see various size in percentage, is there a reason for it?

reficul31 commented 7 years ago

I have tried my best not to use percentages in sizes. I think now whatever percentage sizing being used is necessary(according to me). If any mistakes are found I'll change and make the PR again. Thanks for the review! :D

reficul31 commented 7 years ago

@Treora @obsidianart It recently came to my notice that this PR was never merged(Sorry). I have made the PR merge ready again. We can either close it and go with Oliver's approach or I can change the PR again. Thanks.

blackforestboi commented 7 years ago

@reficul31 the main changes I (wanted to make) were regarding the font-size of the different lines, the spaces between elements and some other cleaning up of CSS. See the screenshot.

screen shot 2017-04-02 at 12 08 16

Treora commented 7 years ago

I think Oliver's approach is just this PR plus some changes to the design, but he did not continue from this branch so @reficul31's commits disappeared from the history.

I'd take this branch as it was, rebase onto master (rebasing is cleaner than merging), then apply Olivers changes to it (again a rebase), and end up with a single PR with all your edits combined and attributed to the right authors.

reficul31 commented 7 years ago

I have rebased the master on this branch as per @Treora advice. I have implemented the design by @oliversauter on this branch as well. Please tell me any further changes are required. @oliversauter thanks for the design input. It looks much cleaner now. Although I have left the background as is due to the earlier conversations on this thread. Here is how the final design looks like. screenshot from 2017-04-05 00 11 28 edit: removed the merge conflicts. PR is merge ready. :) Please tell me if any more changes are required in the code or the design.

Treora commented 7 years ago

Rebased and merged (de16361).

@reficul31: One thing to I would like to check still: are the icons you used in the public domain, not copyrighted? Also, are you okay with waiving your copyrights on your contributions? I try to make&keep this repo completely copyright-free.

reficul31 commented 7 years ago

Also, are you okay with waiving your copyrights on your contributions? I try to make&keep this repo completely copyright-free.

Yes, I willingly waive my copyrights on my contributions.

One thing to I would like to check still: are the icons you used in the public domain, not copyrighted?

Yes, they were as far as I remember. The empty screenshot icon was give to me by @oliversauter. I'll do a check on it once.