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.

Sign up for GitHub

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

Jump to bottom

[0.10] Fix table performance & rework attribute(list) logic #504

Open
wants to merge 33 commits into
base: release-0.10
Choose a base branch
from
Open

[0.10] Fix table performance & rework attribute(list) logic #504

wants to merge 33 commits into from

Conversation

v1r0x
Copy link
Member

@v1r0x v1r0x commented Jun 6, 2024

This branch started as a simple performance booster for table attributes. But the whole attribute handling was a huge code block with DRY code in the table component as well.
Thus, I reworked the attributelist and table component to be based on a new Attribute component (tables also has a new TabularRow component). This introduced a new problem with entity attributes (especially in tables), so I tried to move all logic that loads the data needed for the frontend to the serialize and unserialize in the AttributeType classes.

v1r0x added 14 commits June 4, 2024 13:55
@v1r0x
move attribute to own component
1760715 
Signed-off-by: Vinzenz Rosenkranz <vinzenz.rosenkranz@uni-tuebingen.de>
@v1r0x
limit dropdown attribute options and make them infinite for better pe…
471816c 
…rformance

Signed-off-by: Vinzenz Rosenkranz <vinzenz.rosenkranz@uni-tuebingen.de>
@v1r0x
fix several warnings and errors
4b5aeaa 
Signed-off-by: Vinzenz Rosenkranz <vinzenz.rosenkranz@uni-tuebingen.de>
@v1r0x
handle row events
321328d 
Signed-off-by: Vinzenz Rosenkranz <vinzenz.rosenkranz@uni-tuebingen.de>
@v1r0x
move attribute structure handling to attributetype classes
1306a10 
Signed-off-by: Vinzenz Rosenkranz <vinzenz.rosenkranz@uni-tuebingen.de>
@v1r0x
fix setting entity type search restrictions on table columns
7100a61 
Signed-off-by: Vinzenz Rosenkranz <vinzenz.rosenkranz@uni-tuebingen.de>
@v1r0x
update changelog
645de92 
Signed-off-by: Vinzenz Rosenkranz <vinzenz.rosenkranz@uni-tuebingen.de>
@v1r0x
do not save empty rows in table
fdb99a8 
Signed-off-by: Vinzenz Rosenkranz <vinzenz.rosenkranz@uni-tuebingen.de>
@v1r0x
fix adding new attributes to store
4949289 
Signed-off-by: Vinzenz Rosenkranz <vinzenz.rosenkranz@uni-tuebingen.de>
@v1r0x
fix table row index for correct updating rows
3ccb28f 
Signed-off-by: Vinzenz Rosenkranz <vinzenz.rosenkranz@uni-tuebingen.de>
@v1r0x
update multiselect lib for perf improvements
7b2fd45 
Signed-off-by: Vinzenz Rosenkranz <vinzenz.rosenkranz@uni-tuebingen.de>
@v1r0x
fix several warnings and minor issues
e46ba18 
Signed-off-by: Vinzenz Rosenkranz <vinzenz.rosenkranz@uni-tuebingen.de>
@v1r0x
simply return row index
cb2ce54 
Signed-off-by: Vinzenz Rosenkranz <vinzenz.rosenkranz@uni-tuebingen.de>
@v1r0x
only show row options dropdown if hovered to improve performance
558f359 
Signed-off-by: Vinzenz Rosenkranz <vinzenz.rosenkranz@uni-tuebingen.de>
@v1r0x v1r0x changed the title [0.10[ Fix table performance & rework attribute(list) logic [0.10] Fix table performance & rework attribute(list) logic Jun 19, 2024
Severino
Severino reviewed Jul 2, 2024
View reviewed changes
Copy link

@Severino Severino left a comment

Choose a reason for hiding this comment

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

In current state the creation of a table attribute is not working.

resources/js/components/attribute/Attribute.vue Outdated
disabled: computed(_ => data.value.isDisabled || disabled.value),
value: computed(_ => getValueOrDefault()),
// TODO check for selection need?
selection: computed(_ => getAttributeSelection(data.value.id) || {}),
Copy link

Choose a reason for hiding this comment

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

This is a problem, I think this should return an array as default => ... | [ ]
Throws error when creating table attribute.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, selections should be Arrays 👍

resources/js/components/attribute/Attribute.vue Outdated
Comment on lines 346 to 349
reactTo,
hideLinks,
preview,
previewData,
Copy link

Choose a reason for hiding this comment

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

Those are not used.

Copy link
Member Author

Choose a reason for hiding this comment

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

These are some remains from stuff I copy&wasted 😅 can be deleted 👍

resources/js/components/attribute/Tabular.vue Outdated
@@ -620,8 +510,8 @@
initialValue: value.value,
});
const state = reactive({
isPreview: computed(_ => previewColumns.value && Object.keys(previewColumns.value).length > 0),
columns: computed(_ => state.isPreview ? previewColumns.value : getAttribute(attribute.value.id).columns),
isPreview: computed(_ => preview.value && previewData.value && Object.keys(previewData.value).length > 0),
Copy link

Choose a reason for hiding this comment

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

Why isn't preview.value check enough?
Maybe a different name if it does more than one thing?

Copy link
Member Author

Choose a reason for hiding this comment

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

I just did a double check to make sure that we enabled preview (preview.value) and have actual data to preview (previewData.value).

Copy link
Member Author

Choose a reason for hiding this comment

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

I think there was a mix up from the old structure. preview and previewData is part of Attribute.vue and Tabular.vue only receives previewColumns. Thus, checking for previewColumns existance and content is enough.

resources/js/components/attribute/Tabular.vue Outdated
class="text-center"
v-if="row.hidden_info"
class="text-muted text-center fs-5 p-2 bg-primary-subtle"
:colspan="Object.keys(state.columns).length + 2"
Copy link

Choose a reason for hiding this comment

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

use computed at this and the next position? (redundancy)

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

resources/js/helpers/helpers.js Outdated
case 'string-sc':
case 'date':
case 'url':
return '';
Copy link

Choose a reason for hiding this comment

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

There are 3 positions where we return ' '. Combine the non-default cases that return ''?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we can add this at the end right before default. Thought I already did that 🤔

@v1r0x
clean up code
c835670 
Signed-off-by: Vinzenz Rosenkranz <vinzenz.rosenkranz@uni-tuebingen.de>
@v1r0x
use bs css class to overwrite bg color
7c474a0 
Signed-off-by: Vinzenz Rosenkranz <vinzenz.rosenkranz@uni-tuebingen.de>
@v1r0x
fix adding tabular attribute
a671111 
Signed-off-by: Vinzenz Rosenkranz <vinzenz.rosenkranz@uni-tuebingen.de>
@v1r0x
trigger hook
97b80f7 
Signed-off-by: Vinzenz Rosenkranz <vinzenz.rosenkranz@uni-tuebingen.de>
@v1r0x v1r0x force-pushed the 0.10-fix-performance-table branch from ecc6cc7 to 97b80f7 Compare August 1, 2024 09:20
v1r0x and others added 3 commits August 1, 2024 14:44
@v1r0x
remove duplicate state key
1321b6b 
Signed-off-by: Vinzenz Rosenkranz <vinzenz.rosenkranz@uni-tuebingen.de>
@v1r0x
Merge branch 'release-0.10' into 0.10-fix-performance-table
d729efc
@v1r0x
add newly introduced si-unit attribute
a177433 
Signed-off-by: Vinzenz Rosenkranz <vinzenz.rosenkranz@uni-tuebingen.de>
@v1r0x v1r0x force-pushed the 0.10-fix-performance-table branch from 255d304 to a177433 Compare August 1, 2024 13:02
@v1r0x
Copy link
Member Author

v1r0x commented Aug 1, 2024

I resolved merge conflicts and added our new si-unit-attribute, that was added to the pre-PR attribute handling logic. Can you also confirm it is still working @Severino ?

@Severino
Copy link

Severino commented Aug 2, 2024

I tried getting it to work, but when I create a new and empty table with an SiUnit Attribute, I cannot add a new line, as it creates an empty object, however the si attribute need a metadata object for the bast_unit and the default_unit

v1r0x and others added 3 commits August 5, 2024 11:22
Severino and others added 9 commits August 5, 2024 16:19
@Severino
Removed log
0bb7461
@Severino
Reuse method of single attribute
f6d9ebc
@Severino
Fixed sc and single entity not being resetable
46d6301
@Severino
Fixed reset
3c224e0
@Severino
Fixed si unit label
2c8599d
@Severino
Fix table expand
597eb8d
@v1r0x
fix formatting entity attribute in table
bd87695 
Signed-off-by: Vinzenz Rosenkranz <vinzenz.rosenkranz@uni-tuebingen.de>
@v1r0x
fix typos
48b5956 
Signed-off-by: Vinzenz Rosenkranz <vinzenz.rosenkranz@uni-tuebingen.de>
@v1r0x
fixed resetting entity in table
9fa039b 
Signed-off-by: Vinzenz Rosenkranz <vinzenz.rosenkranz@uni-tuebingen.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Severino Severino Severino left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@v1r0x @Severino