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

637 - create details expandable component #640

Closed
wants to merge 19 commits into from

Conversation

pmpc94
Copy link
Contributor

@pmpc94 pmpc94 commented Oct 31, 2023

Issue

Dependencies

  • None

Decisions

  • Create a Details Expandable component that can be expandable
  • Allow field customization according to any design by the use of slots

Screenshots

Screen.Recording.2023-11-03.at.09.30.59.mov

Copy link

vercel bot commented Oct 31, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
ripe-components-vue ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 6, 2023 11:08am

@joao-conde joao-conde assigned pmpc94 and unassigned joao-conde Nov 3, 2023
@joao-conde joao-conde added the enhancement New feature or request label Nov 3, 2023
@joao-conde joao-conde removed their assignment Nov 3, 2023
@joao-conde
Copy link
Contributor

Not very familiar with components and I would like @3rdvision to take a look.

Copy link
Contributor

@3rdvision 3rdvision 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 contributing to ripe-components-vue, even after vue3 deprecation and this is mostly collecting dust after new products, there's still a lot of work history just sitting around here 😞

It might be frustrating to go full solo dive into old conventions and quality code standards especially when adding an organism component.

Have you considered extending/reusing the form component and section expandable instead of creating this one? We try to avoid duplicated component code as much as possible.

@NFSS10 can you check this one out since you were the one that wrote most of form.vue code?

@3rdvision 3rdvision requested a review from NFSS10 November 3, 2023 18:00
@3rdvision 3rdvision assigned pmpc94 and unassigned 3rdvision Nov 3, 2023
@pmpc94
Copy link
Contributor Author

pmpc94 commented Nov 6, 2023

@3rdvision very good points, perhaps I should have poked someone beforehand to discuss some important topics about this library 😅

regarding your questions, yes I have considered reusing the Form and Section components but it would still require some changes because they are not really the same comparing with the new design. To avoid modifying these components I decided to create a new one but we could still try to reuse them if you think is the best idea.

Comment on lines 14 to 36
<slot v-bind:name="sectionName">{{ sectionName }}</slot>
</h2>
<icon
class="details-expandable-caret"
v-bind:icon="isExpanded[sectionName] ? 'chevron-up' : 'chevron-down'"
/>
</div>
<div
class="details-expandable-row"
v-for="(value, name, subIndex) in section"
v-bind:key="subIndex"
>
<slot v-bind:name="sectionName + '-label-' + name">
<label-ripe
class="details-expandable-title"
v-bind:text="capitalizeName(name)"
v-bind:font-size="labelFontSize"
/>
</slot>
<div class="details-expandable-value">
<slot v-bind:name="sectionName + '-' + name" v-bind:field-value="value">
<input-ripe v-bind:value="value" />
</slot>
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's avoid adding slots until it's strictly necessary. The reason is because as this is a public package, when we do changes like these we "are writing a contract" and committing to it so let's avoid doing those kind of commitments until it's really needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the reason I added a slot here is to give the user an option to customize the input style if needed. What's your suggestion instead?

Comment on lines 57 to 68
<template v-slot:first_row>
Customized Customer
</template>
<template v-slot:first_row-label-field_name>
name
</template>
<template v-slot:first_row-field_object>
<input-ripe v-bind:style="'width: 50%'" v-bind:value="data.first_row.field_object.property_a" />
</template>
<template v-slot:second_row-field_object>
<input-ripe v-bind:style="'width: 50%'" v-bind:value="data.second_row.field_object.property_c" />
</template>
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't directly pass text to a slot, wrap it in a div.

If you want to only change the values use a prop instead. For example, if you only want to change the text in the header, instead of passing text via a slot, create a prop that allows to change the header text.

If it's a field that is dynamically generated, make the prop that allows to generate that dynamic structure accept some kind of field that would change the header. For example, in this case I think it would be to also allow some kind of field to data that would handle that change.
Check other components for reference. For example: https://github.com/ripe-tech/ripe-components-vue/blob/master/vue/components/ui/atoms/dropdown/dropdown.vue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what if we want to change only certain inputs and not all?

@NFSS10
Copy link
Contributor

NFSS10 commented Nov 6, 2023

@3rdvision very good points, perhaps I should have pinned someone before to discuss some important topics about this library 😅

regarding your questions, yes I have considered reusing the Form and Section components but it would still require some changes because they are not really the same comparing with the new design. To avoid modifying these components I decided to create a new one but we could try to reuse them if you think is the best idea.

The style shouldn't be a factor when deciding to use a component or not, the components are bland on purpose and you can style them however you want.

Only when you need structural changes is when you should consider expanding the component or maybe, if needed, create a new one.

Also, doesn't ripe-pulse already have this kind of behaviour? If yes see how it's being done and replicate it. I may be wrong though 😅

@pmpc94
Copy link
Contributor Author

pmpc94 commented Nov 6, 2023

@NFSS10 thanks for your inputs. Nope, afaik pulse doesn't have this kind of behaviour as of now.

I will change to use the Section Expandable component, which is a fairly straightforward change.

The way the form component is done would raise a lot of complexity when building the object to build the desired component. Considering the following example:
image

We would need to create so many arrays just to pass 1 input per column in the first row. Then another column just for the second row. Four columns for the 3rd row etc...

And then what if we have this:
image

We would need to create multiples arrays with multiple columns whereas in the current component we just pass one object with the fields we want to render and then allow its customization whenever the user wants it via slots

@NFSS10
Copy link
Contributor

NFSS10 commented Nov 6, 2023

@NFSS10 thanks for your inputs. Nope, afaik pulse doesn't have this kind of behaviour as of now.

I will change to use the Section Expandable component, which is a fairly straightforward change.

The way the form component is done would raise a lot of complexity when building the object to build the desired component. Considering the following example: image

We would need to create so many arrays just to pass 1 input per column in the first row. Then another column just for the second row. Four columns for the 3rd row etc...

And then what if we have this: image

We would need to create multiples arrays with multiple columns whereas in the current component we just pass one object with the fields we want to render and then allow its customization whenever the user wants it via slots

If you think the form's complexity is going to affect very negatively and be too over-engineered for the feature then I would recommend to just create the specific forms and inputs combinations in ripe-pulse and use section-expandable for the expandable behaviour.

It will probably be a lot more simple and cleaner doing that way. Creating another component will be redundant for the context of ripe-components-vue

@pmpc94
Copy link
Contributor Author

pmpc94 commented Nov 6, 2023

@NFSS10 hmmm that makes sense as well, thanks for your feedback. @joao-conde what do you think? Personally I like this idea to not create just "another" component in this library but to adapt it to Pulse's needs instead

@joao-conde
Copy link
Contributor

All valid points, seems good to me, I agree with Nuno

@pmpc94 pmpc94 closed this Nov 6, 2023
@NFSS10 NFSS10 deleted the v-pca/637-create-details-expandable-component branch November 6, 2023 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants