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

Accordion icon-position can only be set as an attribute #6199

Closed
paulcpederson opened this issue Dec 29, 2022 · 5 comments · Fixed by #7191
Closed

Accordion icon-position can only be set as an attribute #6199

paulcpederson opened this issue Dec 29, 2022 · 5 comments · Fixed by #7191
Assignees
@geospatialem
Labels
4 - verified Issues that have been released and confirmed resolved. ArcGIS Online Issues logged by ArcGIS Online team members. bug Bug reports for broken functionality. Issues should include a reproduction of the bug. estimate - 3 A day or two of work, likely requires updates to tests. p - medium Issue is non core or affecting less that 60% of people using the library
Milestone
2023 July Priorities

Comments

@paulcpederson
Copy link
Member

Actual Behavior

If you set accordionEl.iconPosition = "start" the icons of the children accordion items will still appear at the end of the accordion.

However, if you use accordionEl.setAttribute("icon-position", "start") or just set it with kebab case in the DOM, it works great.

Expected Behavior

All props should function using either attributes or... props. We generally prefer using the camel case versions as you get much better type hinting for the values.

Reproduction Sample

https://codepen.io/paulcp/pen/VwBaPEd?editors=0010

Reproduction Steps

  1. look at the icon position (will be at the end)
  2. now change the code to use the attribute version
  3. notice the icon moves to the start

Reproduction Version

All

Relevant Info

No response

Regression?

No response

Impact

No response

Esri team

ArcGIS Online
@paulcpederson paulcpederson added bug Bug reports for broken functionality. Issues should include a reproduction of the bug. 0 - new New issues that need assignment. needs triage Planning workflow - pending design/dev review. labels Dec 29, 2022
@github-actions github-actions bot added the ArcGIS Online Issues logged by ArcGIS Online team members. label Dec 29, 2022
@driskull
Copy link
Member

Yeah, this is another reason to refactor using the getElementProp util.

@geospatialem geospatialem mentioned this issue Mar 21, 2023
Closed
12 tasks
@geospatialem geospatialem added this to the 2023 May Priorities milestone Mar 30, 2023
@geospatialem
Copy link
Member

Work should be conducted prior to #6038.

@geospatialem geospatialem added p - medium Issue is non core or affecting less that 60% of people using the library estimate - 3 A day or two of work, likely requires updates to tests. and removed needs triage Planning workflow - pending design/dev review. labels Mar 30, 2023
@Elijbet Elijbet self-assigned this Jun 16, 2023
@Elijbet Elijbet added 1 - assigned Issues that are assigned to a sprint and a team member. 2 - in development Issues that are actively being worked on. and removed 0 - new New issues that need assignment. 1 - assigned Issues that are assigned to a sprint and a team member. labels Jun 16, 2023
@Elijbet Elijbet mentioned this issue Jun 17, 2023
Merged
Elijbet added a commit that referenced this issue Jun 26, 2023
@Elijbet
accordion properties can be set as an attribute as well as a prop tes…
5548bff 
…t coverage for #6199
@geospatialem
Copy link
Member

Reallocating to the July milestone.

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.
@Elijbet Elijbet removed the 2 - in development Issues that are actively being worked on. label Jul 8, 2023
@Elijbet Elijbet added the 3 - installed Issues that have been merged to master branch and are ready for final confirmation. label Jul 8, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jul 8, 2023

Installed and assigned for verification.

@github-actions github-actions bot assigned geospatialem and unassigned Elijbet Jul 8, 2023
@geospatialem geospatialem added 4 - verified Issues that have been released and confirmed resolved. and removed 3 - installed Issues that have been merged to master branch and are ready for final confirmation. labels Jul 25, 2023
@geospatialem
Copy link
Member

Verified in 1.5.0-next.26

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. ArcGIS Online Issues logged by ArcGIS Online team members. bug Bug reports for broken functionality. Issues should include a reproduction of the bug. estimate - 3 A day or two of work, likely requires updates to tests. p - medium Issue is non core or affecting less that 60% of people using the library
Projects
None yet
Milestone
2023 July Priorities
5 participants
@paulcpederson @driskull @alisonailea @geospatialem @Elijbet