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

keep old behaviour in RoutePath when we just have coordinates (and no… #1250

Merged
merged 7 commits into from
Jul 1, 2024

Conversation

jcardus
Copy link
Contributor

@jcardus jcardus commented Jun 28, 2024

… positions)

include name property in both cases.

… positions)

include name property in both cases.
@jcardus
Copy link
Contributor Author

jcardus commented Jun 30, 2024

any comments?

@tananaev
Copy link
Member

I think we should probably split MapRoutePath to component that handles positions and the one that handles just coordinates.

@jcardus
Copy link
Contributor Author

jcardus commented Jun 30, 2024

can you check?

import { map } from './core/MapView';
import { getSpeedColor } from '../common/util/colors';

const MapRoutePath = ({ name, positions, coordinates }) => {
const MapRoutePath = ({ name, positions }) => {
Copy link
Member

Choose a reason for hiding this comment

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

It looks like name is never used. We can probably remove it from here.

'line-width': 2,
},
});
if (name) {
Copy link
Member

Choose a reason for hiding this comment

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

For this one the name is always provided, so we can probably make it mandatory.

@@ -1,28 +1,13 @@
import { useTheme } from '@mui/styles';
import { useId, useEffect } from 'react';
import { useSelector } from 'react-redux';
import { map } from './core/MapView';
Copy link
Member

Choose a reason for hiding this comment

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

Can you please check the warnings and errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have any warnings

@jcardus jcardus requested a review from tananaev June 30, 2024 23:26
const id = useId();

const theme = useTheme();

const reportColor = useSelector((state) => {
Copy link
Member

Choose a reason for hiding this comment

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

Why are we removing this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the color is based on the position speed

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, but it means you're ignoring the settings.

@jcardus jcardus requested a review from tananaev July 1, 2024 17:51
@tananaev tananaev merged commit e15f109 into traccar:master Jul 1, 2024
1 check passed
@jcardus jcardus deleted the speed-based-color branch July 2, 2024 02:39
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