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

Hide read only embedded entities #6056

Closed
wants to merge 7 commits into from

Conversation

matborowczyk
Copy link
Collaborator

Description

From now on Composer won't display embedded entities with modifier set to "r"

on recording allocated entity is "r"
https://github.com/user-attachments/assets/24086e48-887f-47ba-9b41-01c06fbafeba

closes #6034

Self Check:

Strike through any lines that are not applicable (~~line~~) then check the box

  • Attached issue to pull request
  • Changelog entry
  • Code is clear and sufficiently documented
  • Sufficient test cases (reproduces the bug/tests the requested feature)
  • Correct, in line with design
  • End user documentation is included or an issue is created for end-user documentation (add ref to issue here: )

@matborowczyk matborowczyk self-assigned this Nov 18, 2024
Copy link
Collaborator

@LukasStordeur LukasStordeur left a comment

Choose a reason for hiding this comment

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

This is just a small portion of the code, but already, we are iterating through the same thing multiple times, or that's the impression I get. I don't have the overview picture of the code, but I get the impression we are looping over a lot, multiple times in different places, probably could be unified to keep it more optimized at some point

presentedAttr,
isBlockedFromEditing,
);
embeddedEntity.embedded_entities
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you please add to the comments in the code the reason why we are filtering these out.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added

Copy link
Collaborator

Choose a reason for hiding this comment

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

de-cluttering is good but doesn't tell why we are doing this, I vaguely know it's about the alocated data, but I'd like to see an actual explanation on it why they don't need to be displayed.

@matborowczyk
Copy link
Collaborator Author

This is just a small portion of the code, but already, we are iterating through the same thing multiple times, or that's the impression I get. I don't have the overview picture of the code, but I get the impression we are looping over a lot, multiple times in different places, probably could be unified to keep it more optimised at some point

I understand, and maybe that's true in some places, I think that I would like to iteratively improve the code, especially as we moved out from more complex logic scenario, and I think there is a room for simplification, maybe going back to single source of data.

Here for example the code look similar as we are iterating through embedded entities and filtering out read-only and with the rest we doing something, but that something is different and it's critical here.

There are completely different places, first is for rendering of canvas entities, second one is for stencil/left-sidebar elements, I don't see a value gained from having it abstracted especially we use base array functions there.

After High/Medium and visual Low priority task will be done maybe I will try to look at the code and find something to simplify, or try PoC for one form state where entities have paths, and restrictions for inter-service relations.
That's something that came to my mind few weeks ago, but It's just a rough idea

@matborowczyk matborowczyk added the merge-tool-ready This ticket is ready to be merged in label Nov 21, 2024
@inmantaci
Copy link
Contributor

Processing this pull request

@inmantaci
Copy link
Contributor

Merged into branches master in 0d5641a

@inmantaci inmantaci closed this Nov 21, 2024
inmantaci pushed a commit that referenced this pull request Nov 21, 2024
…oser to make the view cleaner (Issue #6034, PR #6056)

# Description

From now on Composer won't display embedded entities with modifier set to "r"

on recording allocated entity is "r"
https://github.com/user-attachments/assets/24086e48-887f-47ba-9b41-01c06fbafeba

closes #6034

# Self Check:

Strike through any lines that are not applicable (`~~line~~`) then check the box

- [ ] Attached issue to pull request
- [ ] Changelog entry
- [ ] Code is clear and sufficiently documented
- [ ] Sufficient test cases (reproduces the bug/tests the requested feature)
- [ ] Correct, in line with design
- [ ] End user documentation is included or an issue is created for end-user documentation (add ref to issue here: )
@inmantaci inmantaci deleted the issue/6034-readonly-embedded branch November 21, 2024 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-tool-ready This ticket is ready to be merged in
Projects
None yet
Development

Successfully merging this pull request may close these issues.

hide all "r" embedded entities from the view in all modes
3 participants