Skip to content
This repository has been archived by the owner on Oct 30, 2020. It is now read-only.

Search rows #21

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Search rows #21

wants to merge 13 commits into from

Conversation

jboning
Copy link
Contributor

@jboning jboning commented Feb 26, 2014

@jboning
Copy link
Contributor Author

jboning commented Feb 26, 2014

I just realized I never fixed the corp/alliance/personal visibility indicators to work in the new layout. I will get to this tomorrow or something. Still, take a look :)

@Artefact2
Copy link
Member

A couple of remarks:

  • Please don't put useless commits in the history, like 7b121f3. You can rewrite history and trash them before making the pull request.
  • Don't put colors in main sass files, colors go in themes/dark.scss and themes/light.scss. Also use hsl() instead of rgb(), it makes it easier to finetune them by hand.
  • I'm sure people will complain that they want the old layout back (it has its benefits), having the choice between grid/table layout in search results would be nice.
  • Also, default skillset stuff was properly implemented in staging, see 9a226d9#diff-12e55340bd26b6028d53601477fef6d9R1245
  • <table start= is invalid HTML.
  • Always explicitely use <tbody> (and optionally <thead>/<tfoot>) in tables, this is to make things less confusing in XHTML5.
  • Vendor-specific CSS properties should go before the standard property, for future compatibility (this is minor).
  • Last (and not least!), don't rely /only/ on color for conveying information (flyability in your case). This is bad accessibility. See GUIDELINES.md.

@jboning
Copy link
Contributor Author

jboning commented Feb 26, 2014

Bullet by bullet:

  • Sorry about the trashy history. I can fix it and push -f if you want :)
  • Since the colors are semitransparent, they actually work okay the light theme too: https://www.dropbox.com/s/5avukxpm0ha4um4/Screenshot%202014-02-26%2009.13.57.png But if you want to be strict about this for code style's sake, I'm fine with changing that. (I guess without a skillset to check flyability it's not so great: https://www.dropbox.com/s/xhak24m0qq5w2si/Screenshot%202014-02-26%2009.17.14.png Also now I notice that ship names don't show up. Needs some work.)
  • Hmmm. I know it's slightly less information dense (worse comparatively as the screen gets wider), but I feel pretty strongly that the information is easier to consume this way. The design could probably use some tweaks, but I'd like to hear what you think the major benefits are to the old way (not being snarky here--I actually do!).
  • Whoops, guess I've got my head under a rock. I will merge staging and update this to use that when I've got a chance.
  • Whoops. No excuse for this one; must've left it from the old list tag. Will fix.
  • Whoops, bad at (x)html, will fix.
  • Whoops, bad at (s)css, will fix.
  • Finally, a point I can push back on! If you can't fly a fit, I included a section below the fit name showing number of SP until being able to sit in the hull (if relevant) and until being able to fly the whole fit. So the colors are indeed redundant :)

I should be able to get to the various "will fix" things tonight (or I guess that's tomorrow morning for you).

@jboning
Copy link
Contributor Author

jboning commented Feb 26, 2014

P.S. If you take nothing else from this, please please take the ship names on images: https://www.dropbox.com/s/7u6iswce14s8rmr/Screenshot%202014-02-26%2009.47.09.png That will work just as well outside the row design, and is a huge usability improvement. I'll probably break that out into a separate and hopefully less contentious pull request tonight as well.

@Artefact2
Copy link
Member

There's no rush, I will likely not merge this until I am done with porting stuff to DOM, and that is a lot of work. So you still have some time to polish things up.

I'm actually not a fan of the background color covering the entire row, in my (humble) opinion it feels very intrusive and eye-catching (too much perhaps). Maybe a skill icon with a colored background, or using a background-color for only one <td> would be better suited.

About the grid vs table, they both have their ups and downs, but Osmium used to have a table layout that looked like yours a long time ago (see some old screenshots of version 0.6.0). People didn't really like it. Also, the space difference is all but small with 1920x1080.

@jboning
Copy link
Contributor Author

jboning commented Feb 26, 2014

Yeah, the flyability styling could definitely stand to be played around with, I certainly haven't explored all of the design space for that. I'd argue in favor of it being very noticeable, though. As a player looking at a list of fits, the top thing affecting whether a result is useful to me is whether I can fly it.

Good to know about the historical context on tables vs grid. Looking at those screenshots, I think the current grid has a couple of big advantages over the table:

  • Shows useful information. If I am glancing over a bunch of results, the top things I care about are whether I can use the fit and what its base stats are. (Aside: showing reps instead of dps for logi would be cool; working cap stability in would also be cool.) Votes, tags, author are a mixture of "maybe I'll glance at that" and "might be useful if voting/reputation systems gain momentum".
  • Doesn't waste so much screen real estate; the 0.6.0 table literally uses the left half of the screen.

I think the "useful information" complaint about the old table doesn't apply to this one, since it brings over the information from the grid. The screen real estate complaint might still be valid. At least it's not quite so lopsided as the old one was :) I'm not sure that's enough though.

Were there other complaints about the old table that I'm missing?

I'm now thinking I should break the skill display stuff and the table stuff into separate changes as well. This really did roll a lot of things together that don't need to be

@Artefact2
Copy link
Member

Commit e0bb15b should be interesting for you. I've finally moved the awful formatting code in inc/search.php to DOM.

Here's how I would implement a tabular layout:

  • Make render functions in dom-format-loadout.php similar to formatLoadoutGridLayout and makeLoadoutGridLayout that deal with tables. You can probably refactor most of the code in makeLoadoutGridLayout too.
  • In \Osmium\Search\make_pretty_results, also generate a simple way to choose between the grid and the table layout (I'm thinking about two small-ish self-explanatory icons, but pick whatever you think is best), then use it to determine whether to call makeLoadoutGridLayout or the other one that outputs a table.

Now, formatLoadoutGridLayout will cache its result, so obviously you can't generate skill info for the current character right away and put that in the shared cache. There's probably multiple ways of doing it, but I've though about this one:

  • In formatLoadoutGridLayout, generate a custom element with the minimum skill prerequisites of fitted types (for example, <o-prerequisites :1="typeid1,typeid2,..." :2="typeid1,..." /> etc. Since this is not character-specific, this is safe to cache.
  • In the function rendering o-prerequisites, use whatever skillset should be used and alter the DOM to reflect skill prerequisites. This is a fairly lightweight operation (we don't even need to fetch the loadout with get_fit()).

Anyway, I'm sorry for messing up most of your code this late, but if you have a simpler idea feel free to experiment and let me know!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants