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

Adding historical data table view for all types of subjecs #662

Merged
merged 22 commits into from May 23, 2022

Conversation

JoshuaVulcan
Copy link
Collaborator

https://allenai.atlassian.net/browse/DAS-8038
https://das-8038.pamdas.org

Now that the "historical data" button is available for all subject types, this neatly belongs in the SubjectControls component. As such, lots of code de-duplication and refactoring was necessary.

Copy link
Contributor

@amicavi amicavi left a comment

Choose a reason for hiding this comment

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

Some comments, also I wasn't able to find the new button in the popovers:
image

also, the messages image button it's a little too big for it container:
image

Copy link
Contributor

@luixlive luixlive left a comment

Choose a reason for hiding this comment

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

I think Patsy did a great job on the UI side of this PR. I just added some questions related to the code.

src/SubjectControls/button.js Show resolved Hide resolved
src/SubjectControls/button.js Outdated Show resolved Hide resolved
src/SubjectControls/index.test.js Show resolved Hide resolved
src/SubjectHistoryButton/index.test.js Outdated Show resolved Hide resolved
src/SubjectHistoryButton/index.test.js Show resolved Hide resolved
src/SubjectHistoryButton/styles.module.scss Show resolved Hide resolved
src/TrackToggleButton/index.js Show resolved Hide resolved
src/socket/index.js Show resolved Hide resolved
@luixlive
Copy link
Contributor

Hey @JoshuaVulcan did you miss pushing changes? I see a few comments with no response or update 🤔

Copy link
Contributor

@luixlive luixlive left a comment

Choose a reason for hiding this comment

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

This is ready overall, nothing to block it. Still I added a couple comments 👍

src/StaticSensorsLayer/index.js Outdated Show resolved Hide resolved
src/StaticSensorsLayer/index.js Show resolved Hide resolved
src/SubjectHistoryButton/index.js Outdated Show resolved Hide resolved
Copy link
Contributor

@amicavi amicavi left a comment

Choose a reason for hiding this comment

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

I'm seeing some styles that look broken:
for example the Heatmap button but only in tabs:
image

some missing padding at the end of the text:
image

also for the patrol list, the icons have different sizes:
image

I'm also trying to run my local but is tacking so much time :/

Copy link
Contributor

@amicavi amicavi left a comment

Choose a reason for hiding this comment

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

the historical data button looks bigger than others:
image
image

some paddings and message image is a little too big
image

but the weird thing is I only see the heatmap button being cut in the environment but not in my local, I think the environment could have some missing updates:
image

👉 I only consider as a blocker the size of the track control in the patrols list

@JoshuaVulcan
Copy link
Collaborator Author

JoshuaVulcan commented May 19, 2022

the historical data button looks bigger than others: image image

some paddings and message image is a little too big image

but the weird thing is I only see the heatmap button being cut in the environment but not in my local, I think the environment could have some missing updates: image

👉 I only consider as a blocker the size of the track control in the patrols list

@amicavi Working on the other funk, but the overflow issue with the track controls in the list has been fixed 😎

@JoshuaVulcan JoshuaVulcan requested a review from amicavi May 19, 2022 21:39
Copy link
Contributor

@amicavi amicavi left a comment

Choose a reason for hiding this comment

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

LGTM

@Alcoto95
Copy link
Contributor

Hey! @JoshuaVulcan

@amicavi helped me on testing this one, we were able to see the historical data for both Stationary and Non-Stationary subjects.

The only thing that seems to be wrong is the icon for Historic Data (see image below):

image

It has the same icon as Track on. And the padding seems to be a bit off as well, on the left side.

@JoshuaVulcan
Copy link
Collaborator Author

@Alcoto95 woops! Good catch, I'll get that updated

@JoshuaVulcan
Copy link
Collaborator Author

@Alcoto95 @YazzMagana this has been updated, and those updates were related to minor aesthetic fixes. Can we consider this merge-able so it can go out with the sprint?

@JoshuaVulcan JoshuaVulcan merged commit dc9140a into develop May 23, 2022
@JoshuaVulcan JoshuaVulcan deleted the DAS-8038 branch May 23, 2022 16:10
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.

None yet

4 participants