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

Refactor getElementProp across child components as an outdated pattern. #6038

Closed
26 of 27 tasks
Elijbet opened this issue Dec 14, 2022 · 2 comments
Closed
26 of 27 tasks

Refactor getElementProp across child components as an outdated pattern. #6038

Elijbet opened this issue Dec 14, 2022 · 2 comments
Assignees
@geospatialem
Labels
4 - verified Issues that have been released and confirmed resolved. estimate - 8 Requires input from team, consider smaller steps. low risk Issues with low risk for consideration in low risk milestones p - medium Issue is non core or affecting less that 60% of people using the library refactor Issues tied to code that needs to be significantly reworked.
Milestone
2023 October Prio...

Comments

@Elijbet
Copy link
Contributor

Elijbet commented Dec 14, 2022 •
edited by jcfranco
Loading

Description

Refactor getElementProp across child components as an outdated pattern.

Parents should be setting props on the child if it’s a tightly coupled component pair. With this current pattern if the parent prop changes the child doesn't know about the changes to the parent.

For all relevant components props are passed down and children no longer look up for the prop. To achieve this the parent gets a mutation observer to modify children where necessary.

Proposed Advantages

  • Bring consistency to child styling, where there is a tightly coupled component pair.
  • Children are in sync with parent props.
  • Independent components retain autonomy, eg. a large-scale modal with small-scale child components.

Which Component

All coupled components: where the child shares their parent’s name, like list/list-item, flow/flow-item, combobox/combobox-item.

  • accordion-item
    • iconType
    • iconPosition
    • scale
    • selectionMode
  • combobox-item
    • scale
    • selectionMode
  • combobox-item-group
    • scale
  • dropdown-group
    • scale
  • dropdown-item
    • scale
    • selectionMode
  • segmented-control-item
    • scale
    • appearance
    • layout
  • stepper-item
    • icon
    • numbered
    • layout
    • scale
  • tab-title
    • position
    • scale
@Elijbet Elijbet added refactor Issues tied to code that needs to be significantly reworked. 0 - new New issues that need assignment. needs triage Planning workflow - pending design/dev review. labels Dec 14, 2022
@Elijbet Elijbet changed the title Refactor getElementProp across child components as an outdated pattern. Refactor getElementProp across child components as an outdated pattern. Dec 14, 2022
@Elijbet Elijbet mentioned this issue Dec 14, 2022
Closed
16 tasks
@eriklharper
Copy link
Contributor

eriklharper commented Dec 15, 2022 •
edited
Loading

I've implemented this kind of pattern with calcite-radio-button-group which syncs appropriate child radio-button component props with the parent:

https://github.com/Esri/calcite-components/blob/master/src/components/radio-button-group/radio-button-group.tsx#L106-L117

@Elijbet Elijbet mentioned this issue Dec 15, 2022
Closed
@Elijbet Elijbet added 2 - in development Issues that are actively being worked on. and removed 0 - new New issues that need assignment. labels Dec 15, 2022
@Elijbet Elijbet self-assigned this Dec 15, 2022
@Elijbet Elijbet mentioned this issue Dec 29, 2022
Closed
@geospatialem geospatialem added this to the 2023 May Priorities milestone Dec 29, 2022
@geospatialem geospatialem added 0 - new New issues that need assignment. and removed 2 - in development Issues that are actively being worked on. needs triage Planning workflow - pending design/dev review. labels Dec 29, 2022
@jcfranco jcfranco mentioned this issue Jan 28, 2023
Merged
@jcfranco jcfranco mentioned this issue Mar 21, 2023
Closed
12 tasks
@geospatialem geospatialem mentioned this issue Mar 30, 2023
Closed
@geospatialem geospatialem added p - medium Issue is non core or affecting less that 60% of people using the library estimate - 8 Requires input from team, consider smaller steps. labels May 1, 2023
This was referenced May 18, 2023
Merged
Closed
@Elijbet Elijbet self-assigned this Jun 16, 2023
@Elijbet Elijbet added 1 - assigned Issues that are assigned to a sprint and a team member. and removed 0 - new New issues that need assignment. labels Jun 16, 2023
@Elijbet Elijbet mentioned this issue Jun 17, 2023
Merged
@Elijbet Elijbet added 2 - in development Issues that are actively being worked on. and removed 1 - assigned Issues that are assigned to a sprint and a team member. labels Jun 17, 2023
Elijbet added a commit that referenced this issue Jul 8, 2023
@Elijbet
fix(accordion, accordion-item): icon-position, icon-type, `select…
2b09aba 
…ion-mode` and `scale` can now be set as props or attributes (#7191)

**Related Issue:** #6199, #6038, #6200

## Summary

This should resolve #6199, where you're able to set
`accordionEl.setAttribute("icon-position", "start")`, but not
`accordionEl.iconPosition = "start"`, happening because of a logical
issue within `getElementProp` function.

`getElementProp` is being refactored across child components as an
outdated pattern in #6038, so this will also kick off that issue as
well.

Instead of the `accordion-item` looking up the parent for `iconPosition,
iconType, selectionMode, or scale`, these get set by the `accordion`
parent.

The logic for setting these props thus moves to the parent, getting rid
of the `getElementProp` altogether. The parent component gets a
`mutation observer` to do this as well as `watchers` for when it needs
to modify the children.

With the `mutationObserver` added, there is no longer a need for the
`calciteInternalAccordionItemRegister` event logic.
@geospatialem geospatialem mentioned this issue Jul 19, 2023
Closed
@geospatialem geospatialem changed the title Refactor getElementProp across child components as an outdated pattern. Refactor parent components scale from ancestors Jul 19, 2023
@Elijbet Elijbet mentioned this issue Aug 22, 2023
Merged
2 tasks
Elijbet added a commit that referenced this issue Aug 23, 2023
@Elijbet
refactor(combobox, combobox-item, combobox-group): getElementProp i…
f8a1b91 
…s refactored out across child components as an outdated pattern in favor of inheritable props set directly on parent (#7562)

**Related Issue:** #6038

## Summary
`getElementProp` is refactored out across child components as an
outdated pattern in favor of inheritable props set directly on the
parent.

Instead of the `combobox-item` and `combobox-group` looking up the
parent for scale, these get set by the combobox parent.

The logic for setting these props thus moves to the parent, getting rid
of the `getElementProp altogether`. The parent component gets a mutation
observer to do this as well as watchers for when it needs to modify the
children.

Inherited props addressed:
- [x] `scale`
- [x] `selectionMode`
This was referenced Aug 23, 2023
Merged
Merged
Merged
Elijbet added a commit that referenced this issue Aug 24, 2023
@Elijbet
refactor(dropdown, dropdown-item-group, dropdown-item): `getElementPr…
b5fa7f4 
…op` is refactored out in favor of inheritable props set directly on parent (#7582)

**Related Issue:** #6038

## Summary

`getElementProp` is refactored out across child components as an
outdated pattern in favor of inheritable props set directly on the
parent.

The logic for setting these props thus moves to the parent, getting rid
of the `getElementProp` altogether. The parent component gets a
`mutationObserver` to do this as well as `watchers` for when it needs to
modify the children.

Inherited props addressed:

- [x]  `scale` is inherited from `dropdown`,
- [x] `selectionMode` is inherited from `dropdown-item-group`, so there
can be a mix of types within one dropdown.
Elijbet added a commit that referenced this issue Aug 25, 2023
@Elijbet @benelan
refactor(segmented-control, segmented-control-item): getElementProp…
66f8eff 
… is refactored out in favor of inheritable props set directly on parent (#7592)

**Related Issue:** #6038

## Summary
`getElementProp` is refactored out across child components as an
outdated pattern in favor of inheritable props set directly on the
parent.

Instead of the `segmented-control-item` looking up the parent for
`appearance`, `layout`, and `scale`, these get set by the
`segmented-control` parent.

The logic for setting these props thus moves to the parent, getting rid
of the `getElementProp` altogether. The parent component gets a
`mutationObserver` to do this and watchers for when it needs to modify
the children.

Inherited props addressed:

- [x] appearance
- [x] layout
- [x] scale

---------

Co-authored-by: Ben Elan <no-reply@benelan.dev>
@Elijbet Elijbet mentioned this issue Aug 25, 2023
Merged
2 tasks
@Elijbet Elijbet changed the title Refactor parent components scale from ancestors Refactor getElementProp across child components as an outdated pattern. Aug 25, 2023
jcfranco added a commit that referenced this issue Aug 31, 2023
@Elijbet @driskull
@github-actions @jcfranco @anveshmekala
refactor(stepper, stepper-item): getElementProp is refactored out i…
fc2c720 
…n favor of inheritable props set directly on parent (#7593)

**Related Issue:** #6038

## Summary
`getElementProp` is refactored out across child components as an
outdated pattern in favor of inheritable props set directly on the
parent.

Instead of the `stepper-item` looking up the parent for `icon, layout,
numbered, and scale`, these get set by the `stepper` parent.

The logic for setting these props thus moves to the parent, getting rid
of the `getElementProp` altogether. The parent component gets a
`mutationObserver` to do this as well as watchers for when it needs to
modify the children.

Inherited props addressed:

- [x] icon
- [x] layout
- [x] numbered
- [x] scale

---------

Co-authored-by: Matt Driscoll <mdriscoll@esri.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: JC Franco <jfranco@esri.com>
Co-authored-by: Anveshreddy mekala <anv11827@esri.com>
@geospatialem geospatialem added the low risk Issues with low risk for consideration in low risk milestones label Oct 2, 2023
Elijbet added a commit that referenced this issue Oct 30, 2023
@Elijbet
refactor(tabs, tab, tab-nav, tab-title): getElementProp is refactor…
e070d7d 
…ed out in favor of inheritable props set directly on parent (#7605)

**Related Issue:** #6038

## Summary

`getElementProp` is refactored out across child components as an
outdated pattern in favor of inheritable props set directly on the
parent. Instead of the `tab, tab-title, tab-nav` looking up the parent
for `position` and `scale`, these get set by the `tabs` parent.

The logic for setting these props thus moves to the parent, getting rid
of the `getElementProp` altogether. The parent component gets a
`mutationObserver` to achieve this and watchers for when it needs to
modify the children. Child attribute selectors are subbed with new
classes added dynamically to reflect the scale and position, as
attributes are no longer reflected to be targeted.


Inherited props addressed:

- [x] position
- [x] scale
@geospatialem geospatialem added 4 - verified Issues that have been released and confirmed resolved. and removed 2 - in development Issues that are actively being worked on. labels Oct 30, 2023
@geospatialem geospatialem assigned geospatialem and unassigned Elijbet Oct 30, 2023
@geospatialem
Copy link
Member

Verified in the main branch. 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@geospatialem geospatialem

Labels
4 - verified Issues that have been released and confirmed resolved. estimate - 8 Requires input from team, consider smaller steps. low risk Issues with low risk for consideration in low risk milestones p - medium Issue is non core or affecting less that 60% of people using the library refactor Issues tied to code that needs to be significantly reworked.
Projects
None yet
Milestone
2023 October Priorities - Maintenance...
Development

No branches or pull requests
4 participants
@eriklharper @alisonailea @geospatialem @Elijbet