terriajs icon indicating copy to clipboard operation
terriajs copied to clipboard

Tsify mapColumn

Open zoran995 opened this issue 1 year ago • 2 comments

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 avatar Aug 08 '22 09:08 zoran995

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

steve9164 avatar Aug 29 '22 13:08 steve9164

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

steve9164 avatar Aug 29 '22 13:08 steve9164

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar May 04 '23 11:05 CLAassistant

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

staffordsmith83 avatar May 24 '23 00:05 staffordsmith83

@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

staffordsmith83 avatar Jul 14 '23 05:07 staffordsmith83

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

zoran995 avatar Jul 17 '23 11:07 zoran995

  • 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 👍

zoran995 avatar Jul 18 '23 08:07 zoran995

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 avatar Jul 20 '23 02:07 staffordsmith83

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

staffordsmith83 avatar Aug 02 '23 03:08 staffordsmith83

@steve9164 to review before merging

staffordsmith83 avatar Aug 02 '23 04:08 staffordsmith83

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

staffordsmith83 avatar Sep 07 '23 06:09 staffordsmith83

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

staffordsmith83 avatar Sep 11 '23 23:09 staffordsmith83

All good, ready to merge

staffordsmith83 avatar Sep 11 '23 23:09 staffordsmith83