Skip to content

Conversation

Koc
Copy link
Contributor

@Koc Koc commented Jul 20, 2025

This PR adds possibility to configure column width on a table level. Closes #35

🔍 Preview

Configure width: image

Column with fixed width:
image

TODO:

  • Add extra texts

@Koc Koc force-pushed the feature/add-min-width-column-setting branch 3 times, most recently from be10905 to fca84dc Compare July 20, 2025 23:14
@Koc Koc marked this pull request as ready for review July 20, 2025 23:14
@Koc Koc requested review from enjeck and blizzz as code owners July 20, 2025 23:14
@Koc Koc force-pushed the feature/add-min-width-column-setting branch from fca84dc to bf5eb16 Compare July 20, 2025 23:35
@enjeck
Copy link
Contributor

enjeck commented Aug 12, 2025

  • Add extra texts

I don't think we need frontend tests for this. Unless you meant something else?

@Koc Koc force-pushed the feature/add-min-width-column-setting branch 8 times, most recently from 758c157 to 0031e9f Compare August 15, 2025 21:00
@Koc
Copy link
Contributor Author

Koc commented Aug 15, 2025

@enjeck I've added Behat tests to make sure that we able to store value of the new property and fetch it back.

PR rebased, conflicts solved, tests added. So, please, review

@Koc Koc force-pushed the feature/add-min-width-column-setting branch 3 times, most recently from 8e9e19a to 54aa82c Compare August 18, 2025 20:16
Copy link
Contributor

@AIlkiv AIlkiv left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. I left a few comments.

@silverkszlo silverkszlo self-requested a review September 3, 2025 09:04
@silverkszlo
Copy link
Contributor

silverkszlo commented Sep 3, 2025

In EditColumn.vue, we create a shallow copy of this.column in data() using

editColumn: Object.assign({}, this.column)

As a result, editColumn.customSettings still points to the same object as this.column.customSettings, so any changes to it will also affect the original prop. Since props should be read-only, we should instead create a deep copy, for example:

editColumn: JSON.parse(JSON.stringify(this.column))

This ensures that modifying editColumn does not mutate the original prop. See also: https://stackoverflow.com/a/52467000 and here: https://dev.to/hkp22/javascript-shallow-copy-vs-deep-copy-examples-and-best-practices-3k0a

Copy link
Contributor

@silverkszlo silverkszlo left a comment

Choose a reason for hiding this comment

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

Looks nice, thanks for that! I’ve only reviewed the frontend code and suggested a few small changes. I’ve tested the UI in Firefox and Chromium, and it generally looks good, with just a few minor issues noted in the comments.

@Koc
Copy link
Contributor Author

Koc commented Sep 5, 2025

@silverkszlo I've updated PR according to your suggestions.

Min width changed to 40 50px. Extracted constants for min/max values. Used json decode for deep copy.

Placehoder contains boundaries
image

Extracted separate property
image

@Koc Koc force-pushed the feature/add-min-width-column-setting branch 2 times, most recently from b957801 to b4013fb Compare September 5, 2025 11:41
@Koc Koc force-pushed the feature/add-min-width-column-setting branch 3 times, most recently from 2cbac98 to 3cc7dd1 Compare September 7, 2025 19:34
@Koc
Copy link
Contributor Author

Koc commented Sep 7, 2025

@enjeck @blizzz please review

@Koc Koc force-pushed the feature/add-min-width-column-setting branch 2 times, most recently from 9d53218 to d0894a2 Compare September 9, 2025 10:48
@enjeck enjeck force-pushed the feature/add-min-width-column-setting branch from d0894a2 to e5edb5a Compare September 12, 2025 06:11
Comment on lines 14 to 15
:style="{
width: col.customSettings?.width ? `${col.customSettings.width}px` : 'auto',
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, should we consider how these fixed widths would be like on mobile devices? Might run into a responsiveness issue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right now it looks acceptable. Here an example with a columns that have width respectfully 50 and 200 pixels.

image

to be honest I don't know what to improve here. I'm not a profy in FE development 🤷‍♂️ . Feel free to suggest code snippets or just commit in current branch.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good to me!

@Koc Koc force-pushed the feature/add-min-width-column-setting branch from e5edb5a to d70415a Compare September 13, 2025 21:02
Koc added 2 commits September 14, 2025 04:56
Signed-off-by: Kostiantyn Miakshyn <[email protected]>
Signed-off-by: Kostiantyn Miakshyn <[email protected]>
@enjeck enjeck force-pushed the feature/add-min-width-column-setting branch from d70415a to 2e4935b Compare September 14, 2025 03:56
Copy link
Contributor

@enjeck enjeck left a comment

Choose a reason for hiding this comment

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

Great, thank you so much!

@enjeck enjeck merged commit 611a9dd into main Sep 14, 2025
60 of 64 checks passed
@enjeck enjeck deleted the feature/add-min-width-column-setting branch September 14, 2025 05:17
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.

Set/rememeber/save column width
4 participants