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

Tsify mapColumn #6494

Merged
merged 18 commits into from
Sep 12, 2023
Merged

Tsify mapColumn #6494

merged 18 commits into from
Sep 12, 2023

Conversation

zoran995
Copy link
Contributor

@zoran995 zoran995 commented Aug 8, 2022

What this PR does

Fixes #

Tsify MapColumn, TerriaViewerWrapper, LocationBar, DistanceLegend, Splitter.

Test me

How should reviewers test this? (Hint: If you want to provide a test catalog item, create a Gist of its catalog JSON, add its url and your branch name to this url: http://ci.terria.io/<branch name>/#clean&<raw url of your gist> , and then paste that in the body of this PR)

Checklist

  • There are unit tests to verify my changes are correct or unit tests aren't applicable (if so, write quick reason why unit tests don't exist)
  • I've updated relevant documentation in doc/.
  • I've updated CHANGES.md with what I changed.
  • I've provided instructions in the PR description on how to test this PR.

@zoran995 zoran995 changed the title Tsify map column Tsify mapColumn Aug 8, 2022
The last tag just before prettier v2 was run
@steve9164
Copy link
Member

Whoops. I messed up the post-prettier tag merge. Fixing it now.

@steve9164
Copy link
Member

Hopefully fixed. Sorry about that. I was just testing some instructions I'm going to post to the discussions forum and missed a step.

@AnaBelgun AnaBelgun mentioned this pull request Nov 22, 2022
6 tasks
@AnaBelgun AnaBelgun mentioned this pull request Dec 2, 2022
16 tasks
@CLAassistant
Copy link

CLAassistant commented May 4, 2023

CLA assistant check
All committers have signed the CLA.

@staffordsmith83
Copy link
Contributor

@staffordsmith83 to assess this and the time it will take this sprint

@staffordsmith83
Copy link
Contributor

@zoran995 Ill push this to a new branch on terriajs repo and work on it there if thats ok? Just merging in latest main etc

@zoran995
Copy link
Contributor Author

zoran995 commented Jul 17, 2023

@staffordsmith83 of course, also feel free to push it directly to this branch if you want

Copy link
Contributor

@staffordsmith83 staffordsmith83 left a comment

Choose a reason for hiding this comment

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

ProgressBar.tsx

ProgressBar.tsx is good, made a couple of small changes

  • css wrapper on line 73
  • mode:boolean on line 19
  • added terria.baseMapContrastColor to dependencies array on line 40

MapColumn.tsx

You have removed chromeVersion checking stuff. Is this no longer necessary?

customFeedbacks declared in props but not used. Reimplement the section from MapColumn.jsx that is commented out in latest main?

MapColumn now includes ProgressBar - where was it before? In StandardUserInterface - why is this moved?

// @ts-ignore on line 59, can we fix this?

Is this functionality retained somewhere? ln 67-69 in MapColumn.jsx in main

css={`
${this.props.viewState.explorerPanelIsVisible &&
"opacity: 0.3;"}
`}

FeedbackItem display has been removed
In MapColumn.jsx this was

<For
each="feedbackItem"
of={this.props.customFeedbacks}
index="i"
>
<div key={i}>{feedbackItem}</div>
</For>

Line 123 the condition used to be If condition={!this.props.viewState.hideMapUi}
Do we need to put this back in?

Compass.tsx

Improper find and replace?

"the way our compass is composed" became
"the way our compassbase is composed"

and in a few other places.

Other changes

Moves MapCredits out of StandardUserInterface and into MapColumn - I think this is fine

SUMMARY

There is some great workhere Zoran to neaten up a lot of things, I have really noticed your work. Replacing passing viewstate and terria in props with hooks, restructuring the code, and the complex work of rewriting key files in TS. We are VERY grateful for your work and sorry that it has taken so long to review. Please have a look at these few things and we hope to merge soon!

@zoran995
Copy link
Contributor Author

  • for the Chrome version my understanding is that it is no longer needed. The underlying bug was resolved 4 years ago in modern development it is like supporting IE 🙂
  • The commented-out code for the feedback button in my understanding was only to render the feedback button in the right part of the map. Still, this part is completely reworked and reimplemented as part of map navigation.
  • I am unsure how to render the custom feedback part (feedback item) and in which case they should be visible. Do you have a way to show them in the current main? If we don't have it then I can just put it there blindly and hope it works correctly. The prop could be deleted as it is just reading a value from customElements which is already sent to MapColumn. Honestly, I completely missed that part I thought that it is somehow related to the button implementation above.
  • ProgressBar move - it's really a long time since I've done this so I don't recall but probably some element stacking or css simplification. And based on the old StandardUserInterface it is already in MapColumn section and it was connected to it
  • Good catch for removed css opacity it should be restored 👍
  • Good catch for Line 123 👍

@staffordsmith83
Copy link
Contributor

staffordsmith83 commented Jul 20, 2023

Hi @zoran995 thanks again for your quick response!

OK for points 1,2, and 4.
Regarding point 3, Ive checked with the team and we are no longer using customFeedback so dont worry about this, Ill make a separate issue to clean up all refs to it :-)

@zoran995 would you like to implement the last two points, or should I?

@staffordsmith83
Copy link
Contributor

Hi @zoran995 thanks again for your quick response!

OK for points 1,2, and 4. Regarding point 3, Ive checked with the team and we are no longer using customFeedback so dont worry about this, Ill make a separate issue to clean up all refs to it :-)

@zoran995 would you like to implement the last two points, or should I?

I have implemented those last two changes, I think this is ready to merge @steve9164

Copy link
Contributor

@staffordsmith83 staffordsmith83 left a comment

Choose a reason for hiding this comment

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

All required changes implemented

@staffordsmith83
Copy link
Contributor

@steve9164 to review before merging

@staffordsmith83
Copy link
Contributor

@steve9164 any luck with this? Keen to help if you need

CHANGES.md Outdated
Comment on lines 242 to 243
- Move map credits to map column so it don't get hidden by chart panel
- [The next improvement]
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @steve9164 and @staffordsmith83

Before this is merged in can you please update the CHANGES file here. Thanks!

@zoran995 zoran995 requested a review from nf-s September 11, 2023 11:36
Copy link
Contributor

@nf-s nf-s left a comment

Choose a reason for hiding this comment

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

Done thanks @zoran995 !

@staffordsmith83
Copy link
Contributor

URGENT: before merging this let me check that this is the right PR with the changes I made for Zoran

@staffordsmith83
Copy link
Contributor

All good, ready to merge

@staffordsmith83 staffordsmith83 merged commit ff3973e into TerriaJS:main Sep 12, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants