fix(app-layout): add pageinfosection component [KHCP-14578] #1888

portikM · 2025-01-10T15:46:58Z

Summary

Addresses: https://konghq.atlassian.net/browse/KHCP-14578

Adds CollapsibleSection component exported by app-layout package

adamdehaven

suggestion: can you add this to the appropriate README documentation in the package?

packages/core/app-layout/src/components/collapsibleSection/CollapsibleSection.vue

adamdehaven · 2025-01-10T15:54:13Z

packages/core/app-layout/src/components/collapsibleSection/CollapsibleSection.vue

+          <div
+            v-if="title"
+            class="collapsible-section-title"
+          >
+            {{ title }}
+          </div>


suggestion: should this be an actual header tag? e.g. h3

Added a titleTag prop

What do you think of having a slot to control this instead of multiple props? Then I can just <template #title><h3 id="bookmarkable-h3">{{ title }}</h3></template>

It is slottable <slot name="header">

There's already a header slot for this. It wraps only the title and description.

This PR was merged early by mistake (I forgot to disable auto-merge). If there is anything to follow up on, please drop comments on #1890

No prob! Umm did you have any thoughts on the props vs slots thing in relation to #1888 (comment) or shall I move it to the other PR? FYI: Its totally fine to say, "I hear your comment, just we don't want to get into it atm", we can discuss another time also.

Can't quite understand what the ask is in that comment.. if you're suggesting we add slot for description then I think it's a bit of an overkill at this stage? We don't really have use-cases for it yet and if one-off cases start appearing header prop and some custom CSS can handle that. If that becomes a common occurrence then I think it's worth adding a slot for that.

Here's my remaining question from this thread:

is there a use-case for slotting the header in the first place? The whole point of the component is to allow for a consistent UI across apps, so if we can remove slots where not needed, that would be ideal TBH

I suppose this is turning into a more general props vs slots comment, so I guess I'm asking whether we should generally default to using slots for these sorts of things instead of props because of the difficulties consumers have when they are just given props to work with. I suppose maybe depends if we are making these components to make the our lives easier or making these components to make consumers lives easier?

I guess theres maybe a lot of opinion here, so happy to finish this up and resolve this convo if nobody has any more to add?

packages/core/app-layout/src/components/collapsibleSection/CollapsibleSection.vue

Co-authored-by: Adam DeHaven <[email protected]>

portikM · 2025-01-10T16:49:33Z

suggestion: can you add this to the appropriate README documentation in the package?

Done

vaibhavrajsingh2001

LGTM, just one comment about the naming

vaibhavrajsingh2001 · 2025-01-10T16:42:48Z

packages/core/app-layout/src/components/collapsibleSection/CollapsibleSection.vue

Having collapsible in the component name might be misrepresentation since the section can be non-collapsible too, so we can end up with a non-collapsible CollapsibleSection component.
How about PageSection, similar to PageHeader component we already have.

Valid point but PageSection doesn't suggest that it can be collapsible, engineers would have to know it is from previous experience in order to know to use this component when they need collapsible section.. not sure what's the right approach here. @adamdehaven thought?

Great point 👏

How about something like PageConfigSection or PageInfoSection; something that conveys the use case?

PageSection is a bit too vague IMO.

Knowing that it can be collapsible is really just a documentation issue.

Renamed to PageInfoSection

How about KSection? Come to think of it how come this isn't being added to kongponents? Feels like it should live over there?

Just noticed we typically prefix with App* in this repo. AppPageConfigSection maybe?

How about KSection? Come to think of it how come this isn't being added to kongponents? Feels like it should live over there?

This is Konnect specific and will likely change over time.

Just noticed we typically prefix with App* in this repo. AppPageConfigSection maybe?

Don't want to use the word config so it doesn't get mixed up with EntityConfigCard but AppPageInfoSection sounds good to me.

This PR was merged early by mistake (I forgot to disable auto-merge). If there is anything to follow up on, please drop comments on #1890

johncowen · 2025-01-10T17:03:06Z

How would I add an icon here?

Do we support collapsible sections with icons?

Maybe this is all TBD, but lemme know!

vaibhavrajsingh2001 · 2025-01-10T17:06:07Z

How would I add an icon here?

Do we support collapsible sections with icons?

Maybe this is all TBD, but lemme know!

For the image you shared, the icon would go in the actions slot and it'll be a non-collapsible section.
If an icon is supposed to be there then it shouldn't be a collapsible section, because where would the chevron go.

johncowen · 2025-01-10T17:20:56Z

For the image you shared, the icon would go in the actions slot and it'll be a non-collapsible section.

Ah missed that, thanks!

portikM · 2025-01-10T17:21:11Z

How would I add an icon here?

Do we support collapsible sections with icons?
Maybe this is all TBD, but lemme know!

For the image you shared, the icon would go in the actions slot and it'll be a non-collapsible section. If an icon is supposed to be there then it shouldn't be a collapsible section, because where would the chevron go.

Just to add to this point: if the section was collapsible but didn't have a chevron, there would be no visual indication to user that it is collapsible. That's why we need the chevron there when it is collapsible.

#1888)" This reverts commit 977c718.

adamdehaven · 2025-01-10T17:32:13Z

packages/core/app-layout/docs/page-info-section.md

@@ -0,0 +1,74 @@
+# PageInfoSection.vue


You didn't update the docs for the naming, etc.

portikM added 2 commits January 10, 2025 10:41

fix(app-layout): add collapsiblesection component [KHCP-14578]

8e0559b

fix: stylelint:fix

f9ab7b8

portikM self-assigned this Jan 10, 2025

portikM requested review from adamdehaven, jillztom and a team as code owners January 10, 2025 15:46

adamdehaven reviewed Jan 10, 2025

View reviewed changes

portikM and others added 3 commits January 10, 2025 11:44

fix: address pr feedback

8a0df80

fix: minor fix

b40cd01

fix: apply pr feedback

267782c

Co-authored-by: Adam DeHaven <[email protected]>

portikM requested a review from adamdehaven January 10, 2025 16:51

vaibhavrajsingh2001 reviewed Jan 10, 2025

View reviewed changes

portikM enabled auto-merge (squash) January 10, 2025 16:56

portikM added 2 commits January 10, 2025 12:11

test(app-layout): fix component tests [KHCP-14578]

db78ca0

fix: address pr feedback

cb05edc

portikM changed the title ~~fix(app-layout): add collapsiblesection component [KHCP-14578]~~ fix(app-layout): add pageinfosection component [KHCP-14578] Jan 10, 2025

vaibhavrajsingh2001 approved these changes Jan 10, 2025

View reviewed changes

portikM merged commit 977c718 into main Jan 10, 2025
8 checks passed

portikM deleted the fix/khcp-14578-collapsible-section-component branch January 10, 2025 17:31

portikM added a commit that referenced this pull request Jan 10, 2025

Revert "fix(app-layout): add collapsiblesection component [KHCP-14578] (

0c320e1

#1888)" This reverts commit 977c718.

adamdehaven reviewed Jan 10, 2025

View reviewed changes

This was referenced Jan 10, 2025

Revert "fix(app-layout): add pageinfosection component [KHCP-14578]" #1889

Closed

fix(app-layout): rename apppageinfosection component #1890

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(app-layout): add pageinfosection component [KHCP-14578] #1888

fix(app-layout): add pageinfosection component [KHCP-14578] #1888

portikM commented Jan 10, 2025

adamdehaven left a comment

adamdehaven Jan 10, 2025

portikM Jan 10, 2025

johncowen Jan 10, 2025

adamdehaven Jan 10, 2025

vaibhavrajsingh2001 Jan 10, 2025

portikM Jan 10, 2025

johncowen Jan 10, 2025 •

edited

Loading

portikM Jan 10, 2025

adamdehaven Jan 10, 2025

johncowen Jan 10, 2025

portikM commented Jan 10, 2025

vaibhavrajsingh2001 left a comment

vaibhavrajsingh2001 Jan 10, 2025

portikM Jan 10, 2025

johncowen Jan 10, 2025

adamdehaven Jan 10, 2025

portikM Jan 10, 2025

johncowen Jan 10, 2025

adamdehaven Jan 10, 2025 •

edited

Loading

adamdehaven Jan 10, 2025

portikM Jan 10, 2025 •

edited

Loading

portikM Jan 10, 2025 •

edited

Loading

johncowen commented Jan 10, 2025

vaibhavrajsingh2001 commented Jan 10, 2025

johncowen commented Jan 10, 2025

portikM commented Jan 10, 2025

adamdehaven Jan 10, 2025

fix(app-layout): add pageinfosection component [KHCP-14578] #1888

fix(app-layout): add pageinfosection component [KHCP-14578] #1888

Conversation

portikM commented Jan 10, 2025

Summary

adamdehaven left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johncowen Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

portikM commented Jan 10, 2025

vaibhavrajsingh2001 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamdehaven Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

portikM Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

portikM Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

johncowen commented Jan 10, 2025

vaibhavrajsingh2001 commented Jan 10, 2025

johncowen commented Jan 10, 2025

portikM commented Jan 10, 2025

Choose a reason for hiding this comment

johncowen Jan 10, 2025 •

edited

Loading

adamdehaven Jan 10, 2025 •

edited

Loading

portikM Jan 10, 2025 •

edited

Loading

portikM Jan 10, 2025 •

edited

Loading