Skip to content
This repository has been archived by the owner on Apr 24, 2024. It is now read-only.

1113 adjust defaults new map defaults #1302

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

andreicristian97
Copy link
Contributor

@andreicristian97 andreicristian97 commented Apr 18, 2024

Summary

To make the default view of a new map make sense for the user, we change the scaling of the map polygon and we move the view rectangle to be centered on (0, 0).

The calculation for the new view rectangle is not ideal, see first comment for description.

PR should not be merged before the approach is discussed and maybe a better solution is proposed.

Basics

  • The PR is rebased with current master
  • I added a line to changelog.md
  • Details of what I changed are in the commit messages
  • References to issues, e.g. close #X, are in the commit messages and changelog
  • The buildserver is happy

Checklist

  • I fully described what my PR does in the documentation
  • I fixed all affected documentation
  • I fixed the introduction tour
  • I wrote migrations in a way that they are compatible with already present data
  • I fixed all affected decisions
  • I added automated tests or a manual test protocol
  • I added code comments, logging, and assertions as appropriate
  • I translated all strings visible to the user
  • I mentioned every code or binary not directly written or done by me in reuse syntax
  • I created left-over issues for things that are still to be done
  • Code is conforming to our Architecture
  • Code is conforming to our Guidelines
  • Code is consistent to our Design Decisions
  • Exceptions to any guidelines are documented

First Time Checklist

Review

  • I've tested the code
  • I've read through the whole code
  • I've read through the whole documentation
  • I've checked conformity to guidelines
  • I've checked conformity to requirements
  • I've checked that the requirements are tested

@andreicristian97 andreicristian97 linked an issue Apr 18, 2024 that may be closed by this pull request
2 tasks
@andreicristian97
Copy link
Contributor Author

Explanation related to how the View rectangle is centered at (0, 0):

  • We need to figure out the coordinates of the top left corner of the map. For this, it is enough to calculate the actual width and height of the visible part of the map
  • We use the window.innerWidth/Height properties as a base for the calculation, from which we need to subtract some values
  • For the width, we need to subtract twice the default width of the Toolbar element (this is currently hardcoded to be 300 inside the component)
  • For the height, we need to subtract the height of the Navbar (64px coming from the height property h-16 equalling 4rems), and the height of the Timeline (this is a bit harder to calculate step-by-step as it is a result of many styles - total height is currently 133px)

In the code we have those values hardcoded for the calculation - this is not ideal but I am not sure how else to approach this, maybe someone else has another solution.

@andreicristian97 andreicristian97 self-assigned this Apr 18, 2024
@andreicristian97 andreicristian97 added help wanted Extra attention is needed please review Review by unspecified person requested labels Apr 18, 2024
@markus2330
Copy link
Contributor

Explanation related to how the View rectangle is centered at (0, 0):

Please put this as code comment. It is nearly impossible for devs to find text in PRs later on.

Copy link
Contributor

@hatchla hatchla left a comment

Choose a reason for hiding this comment

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

I would suggest having a config file of some sort (or adding it to an existing file where it fits) in the map_planning feature maybe for saving these sizes "globally". If not, leaving it like this is also good for now.

  • I've tested the code
  • I've read through the whole code
  • I've read through the whole documentation
  • I've checked conformity to guidelines
  • I've checked conformity to requirements
  • I've checked that the requirements are tested

@andreicristian97
Copy link
Contributor Author

andreicristian97 commented Apr 21, 2024

I changed the approach to the problem. Inside the component, we use a reference to the node of the DOM tree already, so we can use it to grab the height and the width of the "BaseStage" component itself and update it inside the useEffect hook.
It's better than having hard-coded values in the calculation; the only side effect is, if the Timeline loads after this calculation is done, the view rectangle might end up slightly off-center as its height would change when the Timeline renders. (but the difference is relatively small)
Even with the side effect in mind, I think this is better than the old calculation approach

@markus2330
Copy link
Contributor

jenkins build please

@markus2330
Copy link
Contributor

jenkins build please

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
help wanted Extra attention is needed please review Review by unspecified person requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adjust defaults new map defaults
3 participants