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

Epic street editing #989

Draft
wants to merge 53 commits into
base: main
Choose a base branch
from
Draft

Epic street editing #989

wants to merge 53 commits into from

Conversation

kfarr
Copy link
Collaborator

@kfarr kfarr commented Dec 26, 2024

No description provided.

kfarr and others added 28 commits December 24, 2024 20:36
accepts type and index; does not yet accept custom object to represent segment and its constituent components; doesn't appear to apply child components correctly
-- instead look at working code to create new street-segments from an object in street-segment.js

to test, run on console:
> document.querySelector('[managed-street]').components['managed-street'].insertSegment(3, 'bike-lane');
as a user i can detach a clone and the action is undo-able. if i save and reload I cannot undo or reattach the component unless i choose to add a new clone component or reset the segment completely.
…Managed-Street-format

Converting default streets to managed street format
@vincentfretin
Copy link
Collaborator

About the duplicate id issue, and I often see you updating all the ids, you could remove the id from the objects and add them afterwards:

streetLayersData.forEach((entry, index) => { entry.id = index + 1; });

kfarr added 5 commits January 6, 2025 09:47
use central event dispatcher in managed-street instead of mutation observer in each component
also fix up event dispatcher concept
@kfarr
Copy link
Collaborator Author

kfarr commented Jan 6, 2025

About the duplicate id issue, and I often see you updating all the ids, you could remove the id from the objects and add them afterwards:

Thanks did it this way: 3556175

@kfarr kfarr requested a review from vincentfretin January 7, 2025 00:20
Copy link
Collaborator

@vincentfretin vincentfretin left a comment

Choose a reason for hiding this comment

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

Using events is indeed better here. You can probably simplify the code just listening on segment-width-changed on the parent entity, the event bubbles from the segment to the parent that is the managed-street entity.
Be careful to properly bind the listener function and use the same function when you remove the listener.

src/components/polygon-offset.js Outdated Show resolved Hide resolved
src/components/polygon-offset.js Outdated Show resolved Hide resolved
src/components/street-align.js Outdated Show resolved Hide resolved
src/components/street-align.js Outdated Show resolved Hide resolved
src/components/street-ground.js Outdated Show resolved Hide resolved
: 'Unknown';
};

// entity.getAttribute('data-parent-component')
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove comment

console.log('Attaching width change listener to existing segment');
segment.addEventListener(
'segment-width-changed',
this.onSegmentWidthChanged.bind(this)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Bind it before.

addedSegments.forEach((segment) => {
segment.addEventListener(
'segment-width-changed',
this.onSegmentWidthChanged.bind(this)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Bind it in init.

removedSegments.forEach((segment) => {
segment.removeEventListener(
'segment-width-changed',
this.onSegmentWidthChanged.bind(this)
Copy link
Collaborator

Choose a reason for hiding this comment

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

You're not removing anything here. this.onSegmentWidthChanged.bind(this) creates a new function and this function is not a currently registered listener. Just use this.onSegmentWidthChanged here after you bind it in init.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think you need to add/remove a listener for each segment, you can just register the listener on this.el here. The emitted event from segment bubbles to the parent. Verify what you have for event.target, if it's not the segment, add the segment in the event detail and access it with event.detail.el.

src/components/street-align.js Show resolved Hide resolved
this.el.addEventListener('segments-changed', this.realignStreet);

// Initial alignment
this.realignStreet();
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove this call, it's done a second time in the setTimeout

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.

2 participants