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

Excluding the last building on the map results in an error, reincluding the buildings makes them all the same size #901

Closed
RomanenkoVladimir opened this issue Mar 27, 2020 · 4 comments · Fixed by #2297
Assignees
Labels
bug Only issues that describe bugs. difficulty:medium The difficulty to solve this is not super complex but not easy either priority:high Set by PO
Milestone

Comments

@RomanenkoVladimir
Copy link
Member

Bug

Console Error after last building is excluded, Error in Building Size and Color afterwards if buildings are included again

Expected Behavior

When the user excludes the last building the map is empty, reincluded buildings look the same as before exclusion.

Actual Behavior

When the user excludes the last building on the map a console error is thrown, reincluding buidlings afterwards results in all of them being color, due to the metric switching to unary

Steps to Reproduce the Problem

  1. Exclude all buildings
  2. The Web Application throws a TypeError: "reduce of empty array with no initial value"
  3. Reincluding buildings makes them all red, metric switches to unary

Specifications

  • OS: Windows
  • Browser: Firefox, Chrome
@RomanenkoVladimir RomanenkoVladimir added the bug Only issues that describe bugs. label Mar 27, 2020
@alschmut
Copy link
Contributor

alschmut commented Mar 27, 2020

I don't want to defend this behaviour, its not ideal, but I wanna mention that this is strictly not a bug but by design.

  • The root node includes the metric (attributes) of its children. In this case the root node does only contain the fake metric unary, as it has no children.
  • And whenever the available metrics change, the selected metrics reset themselves to the first available metric, which in the case is unary

Maybe this helps understanding the issue :)

@RomanenkoVladimir
Copy link
Member Author

Ah ok, I understood what led to the behaviour but still thought a user would expect a similar color scheme to the one before the exclusion. If this is a decision by design, I was wrong here.

However, the console error still remains.

Should I change the issue or close it?

@alschmut
Copy link
Contributor

Yes you are right, we could think of another solution here. Not sure what you mean with color scheme though, as there is not other available metric nor more than one node ;)

Nah, lets keep the issue as a bug (as it looks like one from a user perspective). But it requires more like a workaround than a fix.

@BridgeAR BridgeAR added the priority:low Set by PO label May 8, 2020
@BridgeAR
Copy link
Member

I had another look at this. There are actually different ways how a user might end up with no nodes at all.

If a user clicks on a building or folder that would end up in having no nodes left by excluding them, I would just open a popup warning that it's not possible to hide all nodes.

Another way to exclude all nodes is by e.g., searching for * and then to use that as an exclude pattern. I would open the same popup that notifies the user that this is not possible.

To do so, we could just check if the exclude operation would result in having no nodes left. If that's the case, open the mentioned notification and prevent the actual operation from succeeding (i.e., the exclude pattern from the search should not be added to the pattern list).

I changed the priority of this issue, since it's pretty easy to run into this and it ends up with very weird behavior afterwards.

@BridgeAR BridgeAR added difficulty:medium The difficulty to solve this is not super complex but not easy either priority:high Set by PO and removed priority:low Set by PO labels Jul 25, 2020
@NearW NearW self-assigned this Aug 5, 2020
NearW added a commit that referenced this issue Aug 12, 2020
#901

Refactor preRenderService for a more controllable data flow
NearW added a commit that referenced this issue Aug 12, 2020
…event it from being open when the error dialog appears #901
NearW added a commit that referenced this issue Aug 12, 2020
NearW added a commit that referenced this issue Sep 3, 2020
…unused list of flattened nodes when checking for the number of excluded ones #901
NearW added a commit that referenced this issue Sep 3, 2020
NearW added a commit that referenced this issue Sep 3, 2020
NearW added a commit that referenced this issue Sep 3, 2020
NearW added a commit that referenced this issue Sep 9, 2020
@BridgeAR BridgeAR added this to the bug-hunt milestone Jan 1, 2021
@Jacob-Kirimi Jacob-Kirimi self-assigned this Mar 12, 2021
Jacob-Kirimi pushed a commit that referenced this issue Apr 19, 2021
Jacob-Kirimi pushed a commit that referenced this issue Apr 19, 2021
Jacob-Kirimi pushed a commit that referenced this issue Apr 19, 2021
Jacob-Kirimi pushed a commit that referenced this issue Apr 19, 2021
Jacob-Kirimi pushed a commit that referenced this issue Apr 19, 2021
@ngormsen ngormsen self-assigned this Jul 9, 2021
ngormsen added a commit that referenced this issue Jul 14, 2021
ngormsen added a commit that referenced this issue Jul 14, 2021
ngormsen added a commit that referenced this issue Jul 14, 2021
ngormsen added a commit that referenced this issue Jul 14, 2021
ngormsen added a commit that referenced this issue Jul 14, 2021
ngormsen added a commit that referenced this issue Jul 14, 2021
ngormsen added a commit that referenced this issue Jul 14, 2021
ngormsen added a commit that referenced this issue Jul 14, 2021
ngormsen added a commit that referenced this issue Jul 14, 2021
ngormsen added a commit that referenced this issue Jul 14, 2021
RomanenkoVladimir pushed a commit that referenced this issue Jul 16, 2021
* Add simple fix #901

* Add error message when excluding last building #901

* Fix console error  message #901

* Fix logic and add check to search #901

* Fix failing tests #901

* Add more tests #901

* Fix test logic #901

* Revert simple fix #901

* Add changelog entry #901

* Add e2e test for context menu #901

* Add e2e test for '*' search #901
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Only issues that describe bugs. difficulty:medium The difficulty to solve this is not super complex but not easy either priority:high Set by PO
Projects
None yet
6 participants