evewspace / eve-wspace

Wormhole mapping and corporation management for Eve Online.
Apache License 2.0
86 stars 51 forks source link

Minor UI improvements #241

Closed proycon closed 7 years ago

proycon commented 8 years ago
raphendyr commented 8 years ago

Nice changes, though it would be cool if the commit list would be bit shorter, e.g. commit "typo" could be squashed. Also those "3rd, attempt".

proycon commented 8 years ago

Yeah, I'm a bit of a commit-early, commit-often type, which tends to lead to this :) I tried to squash some but that kind of messed things up, I'd rather not mess with history...

raphendyr commented 8 years ago

Sure. I recommend learning to use rebase efectively at some point though. It's so much easier to find bugs from projects.

Anycase, better to commit often and early, instead of losing data.

edit: oh and "commit --amend" is handy for small changes, if you don't use git hooks to push to production.

proycon commented 8 years ago

Ok, I squashed a whole bunch of commits now (see above) ...should be better I hope.. assuming nobody pulled this in before now (if so, reset and do again).

raphendyr commented 8 years ago

Cool. Looks way better imho :) Give it 1-2 years and rebase and history cleaning is just normal task :P

raphendyr commented 8 years ago

Btw. Not exatly for this, but could there be addWaypoint button next to setDestination? info system info and saved locations?

proycon commented 8 years ago

There used to be such a button in the destination list if I recall, I may even have been the one who removed it at some point (not in this PR) cause I didn't see the need for it and found it cluttering and confusing :$

raphendyr commented 8 years ago

Well.. I personally check route distances between 2 random entry systems (e.g. 2 maps) setting the first as destination and second as waypoint. The game calculates "N+" number as jumps and the real distance is "N-1". So to find best hisec entry to travel to some other map (e.g. operation) I use that system. Thus the add waypoint would be really handy. Though I think that button can be way smaller than the set destination.

proycon commented 8 years ago

Actually, I implemented a feature for that in my feat_mapoverlap branch (https://github.com/proycon/eve-wspace/tree/feat_mapoverlap). It shows distance between all k-space exits/entries on all maps (and mentions on which map it is). It's not in this PR as it's a bit unpolished still because it doesn't take any map permissions into account.

raphendyr commented 8 years ago

Omg. that would be handy! I hope you get it polished some day.

Still I'm pretty sure that add waypoint might be usefull sometimes. As it's not so ofthen the button could be small or otherwise less space taking. Even if we can get that as context menu for set destination button, that could work.

Or actually the system name. Right click (or left if that is ok) the system name and it would show context menu with actions "show info", "set destination" and "add waypoint".

Zumochi commented 8 years ago

:+1: better than attempting to double click on something ten times before it works.

On Wed, Feb 24, 2016 at 4:47 PM, Jaakko Kantojärvi notifications@github.com wrote:

Omg. that would be handy! I hope you get it polished some day. Still I'm pretty sure that add waypoint might be usefull sometimes. As it's not so ofthen the button could be small or otherwise less space taking. Even if we can get that as context menu for set destination button, that could work.

Or actually the system name. Right click (or left if that is ok) the system name and it would show context menu with actions "show info", "set destination" and "add waypoint".

Reply to this email directly or view it on GitHub: https://github.com/evewspace/eve-wspace/pull/241#issuecomment-188312958

raphendyr commented 8 years ago

Can you rebase this on top of current develop and squash some of the commits at least. Most are just small style changes so I don't see need for them to be separate commits.

proycon commented 8 years ago

Right, I'll try to get onto this in the coming days.

proycon commented 7 years ago

Sorry for the delay, I just rebased my minoruiimprovements branch on the develop branch. I'm a bit confused about all of the older unsquashed commits being part of this PR though, things seem a bit messy... Probably even more messy than before the rebase :/

proycon commented 7 years ago

I undid my rebase again, will try anew

proycon commented 7 years ago

Rebase complete, conflict resolved.

raphendyr commented 7 years ago

I'll test this and merge if fine. Thanks in advance.

raphendyr commented 7 years ago

Also, if you like you can clean up the history and squash some commits if there is no need them to be separate. E.g. commit "Fix regression" could be merged into the commit that created regression.

Maarten28 commented 7 years ago

Tested, looks good except two minor UI issues which I'll put in a new issue. Gonna merge it.