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

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

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@reficul31
Copy link
Contributor

reficul31 commented Mar 6, 2017

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

@Treora

This comment has been minimized.

Copy link
Collaborator

Treora commented Mar 8, 2017

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):

  • more space around the buttons
  • remove clustering, or at least the styles for popping up clustered items on hover
  • change to ellipsis overflow for titles & urls (also they do not get to use the full width for some reason?)
  • "No screenshot available" is not so important, could be very thin and lightgrey, or removed completely.
  • keep the background image until we have a better one

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

@reficul31

This comment has been minimized.

Copy link
Contributor Author

reficul31 commented Mar 8, 2017

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

@reficul31 reficul31 force-pushed the reficul31:issue_40 branch from 37e2bd4 to 69f21ee Mar 8, 2017

@reficul31

This comment has been minimized.

Copy link
Contributor Author

reficul31 commented Mar 8, 2017

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

This comment has been minimized.

Copy link
Collaborator

Treora commented Mar 8, 2017

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 reficul31 force-pushed the reficul31:issue_40 branch from 69f21ee to 5e65c4a Mar 9, 2017

@reficul31

This comment has been minimized.

Copy link
Contributor Author

reficul31 commented Mar 9, 2017

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

This comment has been minimized.

Copy link
Contributor

Chaitya62 commented Mar 10, 2017

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.

@@ -2,7 +2,7 @@
list-style: none;
margin: 40px auto;
padding: 0;
width: 242px;
width: 60%;

This comment has been minimized.

@obsidianart

obsidianart Mar 10, 2017

Collaborator

unless you have a container above in pixel you need min-width too

@@ -1,11 +1,11 @@
.root {
position: relative;
display: block;
width: 240px;
height: 120px;
width: 100%;

This comment has been minimized.

@obsidianart

obsidianart Mar 10, 2017

Collaborator

any particular reason for this? width is auto by default, which means 100% already for most scenarios.

width: 240px;
height: 120px;
width: 100%;
height: 10%;

This comment has been minimized.

@obsidianart

obsidianart Mar 10, 2017

Collaborator

is this 10% of which element? if you don't know probably you want to use wh instead.


.title {
.logo {
height: 100%;

This comment has been minimized.

@obsidianart

obsidianart Mar 10, 2017

Collaborator

as above, are those percentages of which element?

color: #333;
}
.title{
height: 100%;

This comment has been minimized.

@obsidianart

obsidianart Mar 10, 2017

Collaborator

as above, are those percentages of which element?

white-space: nowrap;
margin:0;
padding:0;
height:33.33%;

This comment has been minimized.

@obsidianart

obsidianart Mar 10, 2017

Collaborator

as above, are those percentages of which element?

.screenshot{
float:right;
width:15%;
height:100%;

This comment has been minimized.

@obsidianart

obsidianart Mar 10, 2017

Collaborator

as above, are those percentages of which element?

@obsidianart

This comment has been minimized.

Copy link
Collaborator

obsidianart commented Mar 10, 2017

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

This comment has been minimized.

Copy link
Contributor Author

reficul31 commented Mar 10, 2017

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 reficul31 closed this Mar 10, 2017

@reficul31 reficul31 force-pushed the reficul31:issue_40 branch from 5e65c4a to 83f39b2 Mar 10, 2017

@reficul31 reficul31 reopened this Mar 10, 2017

@reficul31 reficul31 force-pushed the reficul31:issue_40 branch 2 times, most recently from 5189513 to feebe30 Mar 13, 2017

@reficul31

This comment has been minimized.

Copy link
Contributor Author

reficul31 commented Mar 13, 2017

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.

width: auto;
height: auto;
height: 100%;
width: 100%;

This comment has been minimized.

@obsidianart

obsidianart Mar 14, 2017

Collaborator

do you need this?

This comment has been minimized.

@reficul31

reficul31 Mar 14, 2017

Author Contributor

It was observed when the page width was decreased the div with the screenshot was shrinking more when no screenshot was present and less when there was a picture in the div(screenshot was present).
I researched a bit more and found that when the width of the container is decreased in flex, it fits according to the content given in the div. Therefore the height and width are set to 100% for when there is no screenshot the div occupies the same space as that of when there is a screenshot.

@obsidianart

This comment has been minimized.

Copy link
Collaborator

obsidianart commented Mar 14, 2017

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

@reficul31

This comment has been minimized.

Copy link
Contributor Author

reficul31 commented Mar 14, 2017

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 reficul31 force-pushed the reficul31:issue_40 branch from f844f4a to 8341424 Apr 3, 2017

@reficul31

This comment has been minimized.

Copy link
Contributor Author

reficul31 commented Apr 3, 2017

@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.

@oliversauter

This comment has been minimized.

Copy link
Collaborator

oliversauter commented Apr 3, 2017

@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

@reficul31 reficul31 force-pushed the reficul31:issue_40 branch from 8341424 to 8a4d466 Apr 4, 2017

@Treora

This comment has been minimized.

Copy link
Collaborator

Treora commented Apr 4, 2017

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 reficul31 force-pushed the reficul31:issue_40 branch from 8a4d466 to 89ea17a Apr 4, 2017

@reficul31

This comment has been minimized.

Copy link
Contributor Author

reficul31 commented Apr 4, 2017

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.

@reficul31 reficul31 force-pushed the reficul31:issue_40 branch from 58f4b77 to 07e45c4 Apr 5, 2017

@reficul31 reficul31 force-pushed the reficul31:issue_40 branch from 07e45c4 to 7cbe679 Apr 5, 2017

Treora added a commit that referenced this pull request Apr 6, 2017

@Treora

This comment has been minimized.

Copy link
Collaborator

Treora commented Apr 6, 2017

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

This comment has been minimized.

Copy link
Contributor Author

reficul31 commented Apr 6, 2017

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.

@reficul31 reficul31 deleted the reficul31:issue_40 branch Jul 15, 2017

reficul31 pushed a commit to reficul31/webmemex-extension that referenced this pull request Dec 18, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment