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

Chore: Minor fixes & improvements #2232

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Chore: Minor fixes & improvements #2232

wants to merge 10 commits into from

Conversation

maiwald
Copy link
Collaborator

@maiwald maiwald commented Nov 1, 2024

This PR fixes some minor issues that came up while implementing the new navigation structure designed by shapefield. Extracting those issues into a PR so they can be reviewed separately without the navigation restructuring.

@maiwald maiwald marked this pull request as ready for review November 1, 2024 08:02
@maiwald maiwald requested a review from PiTrem November 1, 2024 08:03
@ComPlat ComPlat deleted a comment from github-actions bot Nov 1, 2024
@ComPlat ComPlat deleted a comment from github-actions bot Nov 1, 2024
@maiwald maiwald changed the title Chore: Minor fixes Chore: Minor fixes & improvements Nov 1, 2024
Copy link

github-actions bot commented Nov 1, 2024

LCOV of commit 14508ee during Continuous Integration #4086

Summary coverage rate:
  lines......: 65.5% (14010 of 21400 lines)
  functions..: no data found
  branches...: no data found

Files changed coverage rate: n/a

Copy link

github-actions bot commented Nov 4, 2024

LCOV of commit 7e09543 during Continuous Integration #4093

Summary coverage rate:
  lines......: 65.5% (14010 of 21400 lines)
  functions..: no data found
  branches...: no data found

Files changed coverage rate: n/a

Assigning to `el_composition.data` would cause the sample to be
considered as `isEdited`.
Before this change, we would `this.state.sortColumn = ...` during render
in order to get the latest value from UserStore. Now we properly listen
(and unlisten) for store changes and update the state using
`this.setState`.

With this change we also set the initial UIStore state when the
component mounts and not only when the store state changes.
The ManagingActions component handles syncing genericEls with UserStore
itself. No need to pass that information in as prop.
We used to copy the state to determine whether the selection had changed
or not. However, we can simply calculate that information from the
UIStore state without the local state.
This used to trigger `UIState.handleSelectSyncCollection` which
forwarded to `UIStore.handleSelectCollection`. Removing in favour of
less indirection.
Before this change, we would always reset the currentTab to 0 and not
use the data stored in the UserStore.

This was fine when we never unmounted/mounted the ElementList component
but leads to the undesired effect of not respecting the user's selected
tab when we unmount/mount the component.
This was used to force a re-render when changing to full screen. It does
not seem to be necessary anymore.
Also fix collection type. JS Array has no `size` property.
Copy link

github-actions bot commented Nov 5, 2024

LCOV of commit ddec9b3 during Continuous Integration #4101

Summary coverage rate:
  lines......: 65.5% (14026 of 21400 lines)
  functions..: no data found
  branches...: no data found

Files changed coverage rate: n/a

Copy link

github-actions bot commented Nov 6, 2024

LCOV of commit 81483c8 during Continuous Integration #4111

Summary coverage rate:
  lines......: 65.5% (14010 of 21400 lines)
  functions..: no data found
  branches...: no data found

Files changed coverage rate: n/a

Copy link

github-actions bot commented Nov 6, 2024

LCOV of commit ddec9b3 during Continuous Integration #4113

Summary coverage rate:
  lines......: 65.5% (14012 of 21400 lines)
  functions..: no data found
  branches...: no data found

Files changed coverage rate: n/a

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.

1 participant