Skip to content
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

Updating UI to also fit on mobile devices. Fix #34 #56

Merged
merged 1 commit into from
Jan 23, 2017

Conversation

bagage
Copy link
Collaborator

@bagage bagage commented Nov 27, 2016

I am trying to finish the port for mobile support (#34). So far here are the remaining issues I noted (feel free to edit the list if you find anything else).

  • Elevation graph regressions (no more colors and links with the track).
  • Always show elevation graph on desktop or at least open it by default.
  • Allow profiles to be selected by hotkeys. One solution, as suggested by @nrenner is to move them directly in the navigation bar and remove the Options menu.
  • Missing data tab + future turn instructions display turn instructions #42 and tag graph Display statistics of street surface #45. I think these all three can be skipped on mobile, so maybe a sidebar on desktop would be fine.
  • Investigate the Itinerary control that is missing as well.
  • Fix long profiles name display.
  • Fix (and improve?) Search bar.
  • Elevation button: add active state.
  • Nogo area button: remove cancel button and use the same logic as the Draw button instead. WON'T FIX - actually it is not the same thing (canceling vs finishing).

Currently it looks like this:

capture d ecran de 2016-11-27 22-33-47
Left to do:

  • Fix CSS for profile / alternative dropdown in navigation bar
  • Fix error of updateStyle applied on segment when a node is moved
  • readd missing BR.Control

@nrenner
Copy link
Owner

nrenner commented Nov 28, 2016

Thanks for your contributions! I was going to suggest to move forward with your approach, make it work on mobile first and then see what to do for desktop.

I added you as a collaborator. Depending on how you prefer to work, I would suggest that we merge this PR now and you commit straight to the master branch of the main repo - unless you feel like discussing first. What do you think?

@bagage
Copy link
Collaborator Author

bagage commented Nov 28, 2016

I'd prefer to finish everything before merging it, because many things are broken yet and it does not work really well on mobile. Plus, if you could reread/review the PR before getting merge I would be more comfortable with it :-).

@nrenner
Copy link
Owner

nrenner commented Nov 28, 2016

Ok, fine by me.

@@ -83,6 +81,12 @@
"url-search-params": {
"main": "build/url-search-params.js"
},
"font-awesome": {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

font-awesome seems to be addes twice now (I switched to Font Awesome in b108efa for v0.6.0)?

@bagage
Copy link
Collaborator Author

bagage commented Jan 8, 2017

I'm still working on it - I just managed to make Profile and Alternative appear in the navbar (no CSS yet):

capture d ecran de 2017-01-08 14-13-43

The biggest last part is to reintegrate missing tabs (probably only on desktop though) but also check if we couldn't keep the existent elevation graph instead of rewriting it.

@bagage bagage force-pushed the feature/mobile_support branch 2 times, most recently from a69f721 to f8f3421 Compare January 11, 2017 16:06
@nrenner
Copy link
Owner

nrenner commented Jan 11, 2017

Thanks for the update.

Regarding the elevation graph, I generally prefer using and ideally contributing to external plugins as much as possible. The Leaflet.Elevation code does need some refactoring, but I'm not sure a complete rewrite would be a better solution (but I also don't know D3 well enough to judge).

I guess the main issue was having a responsive graph that adjusts to the screen size, but haven't looked at how hard it would be to add that to the Elevation plugin. Just saw there is some ongoing work in PR #66 - Elastic width that might allow just that?

Let me know if you want a second opinion or some help.

@bagage
Copy link
Collaborator Author

bagage commented Jan 12, 2017

Yeah I agree for the elevation graph - I reverted to Elevation plugin for now, and played a bit with PR66:
There are still a few bugs but I believe it will be OK. Last master missing piece is the ability to hide the elevation graph (which will be required on mobile because it takes way to much place). But again it seems doable.
Yet I am facing only small issues but I'll definitely ask for at least review when I'm filling it ready. Thanks.

@bagage
Copy link
Collaborator Author

bagage commented Jan 12, 2017

OK so I finally was able to move pretty quickly on the remaining items, I'd be happy if you could test and/or review it @nrenner :-).
I'll leave a non-rebased branch in feature/mobile_support_raw if needed. There might be also some dead code and/or forgotten things…

@bagage bagage force-pushed the feature/mobile_support branch from 5c3fd41 to 9939806 Compare January 12, 2017 16:29
Copy link
Collaborator Author

@bagage bagage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left problems

// map.addControl(new BR.Control({
// heading: '',
// divId: 'header'
// }));
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing that has to be fixed. Not sure how though since I don't know what BR.Control is supposed to look like.

m1._routing.nextLine._updateStyle();

console.log("TODO: fixme! //m1._routing.nextLine._updateStyle()");
// m1._routing.nextLine._updateStyle();
Copy link
Collaborator Author

@bagage bagage Jan 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another thing I wasn't able to fix. Due to leaflet upgrade from 0.7.x to 1.x, this function appears not to exist anymore but… any help welcome about how to fix that because I couldn't find the solution so far!

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix in commit Use setStyle instead of _updateStyle doesn't seem to have been merged into the PR branch?

@bagage
Copy link
Collaborator Author

bagage commented Jan 12, 2017

Not as nice as with the alternate elevation version, but it now looks like this:

capture d ecran de 2017-01-12 17-39-28

One major enhancement would be to be able to draw leaflet.elevation graph outside of the map, below it as the other version did. Or at least reduce the bottom offset.

edit: I fixed this.

@aleek
Copy link

aleek commented Jan 13, 2017

Last master missing piece is the ability to hide the elevation graph

I'll need that too. When the elastic width is done, I am planning to CTRL+C CTRL+V the code to also make an elastic height, and on top of this, I'll built the feature to show/hide the elevation graph.

Nevertheless it would be great if we could merge our power to deliver this features!

Copy link
Collaborator Author

@bagage bagage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another left problem.

</ul>
<form class="navbar-form">
<div class="form-group">
<select class="selectpicker show-tick" id="profile-alternative" multiple>
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another thing to fix is the CSS for this form. It should looks like this:

capture d ecran de 2017-01-12 17-42-44

But instead looks like this:

capture d ecran de 2017-01-12 17-42-55

It looks like the CSS is not loaded (gulp problem?) or CSS issue. I'm not much familiar with these so some help would be greatly appreciated here :-). Also it should be probably tweaked a bit to look good on black navbar.

@bagage
Copy link
Collaborator Author

bagage commented Jan 13, 2017

I'll need that too. When the elastic width is done, I am planning to CTRL+C CTRL+V the code to also make an elastic height, and on top of this, I'll built the feature to show/hide the elevation graph.

Yeah for now I'm hacking it a bit:

$('#elevation-btn').on('click', function (event) {
    $('.elevation').is(':visible') ? el.removeFrom(map) : el.addTo(map)
});

But we should definitely work together on these (though I have no much skills :)).

@bagage bagage force-pushed the feature/mobile_support branch 7 times, most recently from c0bd570 to 8cf2e6b Compare January 16, 2017 10:29
@bagage
Copy link
Collaborator Author

bagage commented Jan 16, 2017

OK, I was finally able to fix the remaining problems. This time it is fully ready to be reviewed & merged!

capture d ecran de 2017-01-16 11-29-15

@bagage bagage changed the title [WIP] Updating UI to also fit on mobile devices. Fix #34 Updating UI to also fit on mobile devices. Fix #34 Jan 16, 2017
@nrenner
Copy link
Owner

nrenner commented Jan 16, 2017

Great! It might take me some time to go through this huge PR.

The first obvious thing is that the map doesn't show in Chrome (map height is 0). Can you reproduce this in the Chrome Browser?

@bagage bagage force-pushed the feature/mobile_support branch from 8cf2e6b to d30a8d8 Compare January 16, 2017 15:29
@bagage
Copy link
Collaborator Author

bagage commented Jan 16, 2017

Yeah sorry, forgot to fix that - it's done now.

@nrenner
Copy link
Owner

nrenner commented Jan 18, 2017

Now I see a map, but the 100% map height is taking up the total screen height, so the diagram and the footer are below the visible screen (with scrollbar).

Don't know if related, the Leaflet control buttons have mobile styling applied (larger buttons with border) on desktop in Chrome. Firefox is fine.

(Also see inline comment for missing setStyle commit (others?)).

Still looking into the rest...

@bagage bagage force-pushed the feature/mobile_support branch from d30a8d8 to f6d30cc Compare January 18, 2017 21:01
@bagage
Copy link
Collaborator Author

bagage commented Jan 18, 2017

I need to make more tests for Chrome - I basically did everything on Firefox.
In the mean time only setStyle commit was missing I think, I added it.

@bagage
Copy link
Collaborator Author

bagage commented Jan 23, 2017

OK so I finally rearranged all CSS related to flexbox - it should be OK now!

@bagage bagage force-pushed the feature/mobile_support branch from f6d30cc to d7e476d Compare January 23, 2017 09:49
@nrenner nrenner merged commit a092813 into nrenner:master Jan 23, 2017
@nrenner
Copy link
Owner

nrenner commented Jan 23, 2017

Merging this now, we can work on remaining issues as a follow-up on this.

Thanks a lot for your work @bagage and @RoPP!

@nrenner nrenner mentioned this pull request Jan 30, 2017
15 tasks
@bagage bagage mentioned this pull request Apr 25, 2017
@BikRider
Copy link

BikRider commented May 1, 2017

Will this wonderful interface be available in the online version of brouter? Thx for this gorgeous work :-).

@bagage
Copy link
Collaborator Author

bagage commented May 1, 2017

@BikRider Well there are a few things left (see #74 and #66), but hopefully it should be available soon on the website yes. It should replace the current version totally.

@bagage bagage deleted the feature/mobile_support branch May 5, 2017 13:07
@nrenner nrenner mentioned this pull request Jul 19, 2017
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants