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

Navigation update separate mobile desktop #48

Merged
merged 36 commits into from
Apr 7, 2023

Conversation

JesJehle
Copy link
Collaborator

I made a first version of a separate navigation for mobile and desktop.
The mobile navigation is simple and I used the pages and routes for the map, list, settings and tree details.
The desktop navigation is more complicated and not finished yet. Since we are only working with one page and additional popovers and grids, I created a separate desktop page.
Here I used a grid to display the inventory list and map components, which works after some trial and error.
Now comes the more complicated part.
At the moment settings and treedetails are implemented as pages, to show them in a popup window next to the inventory list, I need to get the routing logic into some kind of state to render the clicked treedetails.
A short talk would be helpful here.
I would rather not merge the current branch, just yes, but please have a look and give feedback. I would implement the changes for the desktop version first and then merge.

@JesJehle JesJehle requested a review from mmaelicke March 31, 2023 12:10
@mmaelicke
Copy link
Member

I added the logic, how the navigation would be used within a single page to show different component. This is also not really working. It feels like we are extremely fighting the framework, trying to do stuff the components were not made for.

I have to investigate a bit more, why the match object supplied by the Router is not updating appropriately.

I am not sure, how we would implement navigation to tree details in mobile, but load the components directly on Desktop.

My last shot are nested router-outlets, which gave me really weird animations in the first place. If that does not work, I guess we need to take a completely different route.

@mmaelicke
Copy link
Member

check https://github.com/hydrocode-de/mathislewald/tree/use-nested-router

this one implements the nested router. Works, kind of, but has a annoying delay.

@mmaelicke
Copy link
Member

Got it. Works with the react-router directly. The IonRouter is made for Pagetransitions, that screwed everything up.
Working with react-router directly is possible, but we need to stick to version 5.2 as the new version (6.X) is not yet supported by ionic.

@JesJehle
Copy link
Collaborator Author

JesJehle commented Apr 3, 2023

Since, the initial draft is done, I would suggest merging this branch to main and continuing from there.
Feel free to merge or request changes.

@JesJehle
Copy link
Collaborator Author

JesJehle commented Apr 6, 2023

To Do:

@JesJehle JesJehle requested review from mmaelicke and removed request for mmaelicke April 7, 2023 11:00
@JesJehle
Copy link
Collaborator Author

JesJehle commented Apr 7, 2023

Ok, I think I am done. @mmaelicke have a look, merge if possible.

Copy link
Member

@mmaelicke mmaelicke left a comment

Choose a reason for hiding this comment

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

Really nice work!

I added one commit to disable the Selection button for now. I will approve, although there are some minor comments. Mainly about re-naming and commenting.

Comment on lines 19 to 33
useEffect(() => {
if (layers.activeBaseLayer.toString() === "density") {
setSrc("assets/density.png");
setTitel("Density");
} else if (layers.activeBaseLayer.toString() === "dtm") {
setSrc("assets/dtm.png");
setTitel("DTM");
} else if (layers.activeBaseLayer.toString() === "ortho") {
setSrc("assets/ortho.png");
setTitel("Ortho");
} else {
setSrc("assets/openstreetmap.png");
setTitel("OSM");
}
}, [layers.activeBaseLayer]);
Copy link
Member

Choose a reason for hiding this comment

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

a comment about the purpose would be nice

Comment on lines 138 to 159
// <IonCard key={f.id}>
// <IonRow class="ion-padding ion-align-items-center">
// <IonCol>
// <IonCardSubtitle>TREE ID</IonCardSubtitle>
// <IonCardTitle>{f.properties.treeid}</IonCardTitle>
// </IonCol>
// {positionEnabled && position ? (
// <IonCol class="ion-text-center">
// <IonCardSubtitle>DISTANCE</IonCardSubtitle>
// <IonCardTitle>
// <IonIcon icon={navigateOutline} size="small" />
// &nbsp; {distString(f)}
// </IonCardTitle>
// </IonCol>
// ) : null}
// <IonCol class="ion-text-center ion-hide-sm-down">
// <IonCardSubtitle>RADIUS</IonCardSubtitle>
// <IonCardTitle>{f.properties.radius.toFixed(1)}</IonCardTitle>
// </IonCol>
// <IonCol class="ion-text-end">
// <IonButton
// // fill="outline"
Copy link
Member

Choose a reason for hiding this comment

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

let's clean this up.

@@ -74,6 +74,7 @@ const MainMap: React.FC = () => {
// 47.884438269626294, 8.088652498339387
}}
>
<div></div>
Copy link
Member

Choose a reason for hiding this comment

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

what is this div for?


interface MapSelectionButtonProps {
name: string;
titel: string;
Copy link
Member

Choose a reason for hiding this comment

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

can we use title ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sure

Copy link
Member

Choose a reason for hiding this comment

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

This component doesn't do anything up to now, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, this one is used in VariableSelectionPopover and VariableSelectionModal.

@JesJehle
Copy link
Collaborator Author

JesJehle commented Apr 7, 2023

Ok, I resolved all issues you pointed out. I will merge this now.

@JesJehle JesJehle merged commit ae93375 into main Apr 7, 2023
@JesJehle JesJehle deleted the navigation-update-seperate-mobile-desktop branch April 7, 2023 13:13
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.

2 participants