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

Remove usage of getElementProp #3757

Closed
12 tasks
benelan opened this issue Dec 21, 2021 · 4 comments
Closed
12 tasks

Remove usage of getElementProp #3757

benelan opened this issue Dec 21, 2021 · 4 comments
Assignees
Labels
1 - assigned Issues that are assigned to a sprint and a team member. Calcite (dev) Issues logged by Calcite developers. epic Large scale issues to be broken up into sub-issues and tracked via sprints and/or project. refactor Issues tied to code that needs to be significantly reworked.
Milestone

Comments

@benelan
Copy link
Member

benelan commented Dec 21, 2021

Summary

Remove usage of the getElementProp util.

getElementProp is misleading from a naming and implementation perspective and needs to be revisited.

Current implementation

export function getElementProp(el: HTMLElement, prop: string, value: any): any {
  const closestWithProp = el.closest(`[${prop}]`);
  return closestWithProp ? closestWithProp.getAttribute(prop) : value;
}
  1. getElementProp reads as a helper to get a prop, but it will actually get an attribute (string) OR whatever fallback value is provided.
  2. The prop argument is used to build the selector for searching. This will only work for single-word props, e.g., will always fail for someName (prop), where the attribute is some-name.
  3. Some usages of the fallback value argument have non-string types. This will lead to a mismatch or confusion on the expected value:
this.aBooleanProp = getElementProp(el, "a-boolean", false); // if a match is found it will return a string (getAttribute)

Proposal

Refactor getElementProp to:

export function getElementProp(el: HTMLElement, prop: string, value: T): T ;

  1. convert the prop name to an attribute when building the selector
  2. look at an element's property
  3. update the type so that the fallback matches the expected return value

Introduce getElementAttr, which would be based on ☝

export function getElementAttr(el: HTMLElement, attr: string, value: string): string ;

  1. use attr arg to build selector
  2. look at an element's attribute
  3. update the return type and fallback value type to be string (matching what getAttribute returns)

Related Issues:

Which Component

  • accordian-item
  • combobox-item
  • combobox-item-group
  • dropdown-group
  • dropdown-item
  • inline-editable
  • input
  • input-message
  • radio-group-item
  • stepper-item
  • tab-title

Other files that reference getElementProp:

  • dom
@benelan benelan added refactor Issues tied to code that needs to be significantly reworked. 0 - new New issues that need assignment. epic Large scale issues to be broken up into sub-issues and tracked via sprints and/or project. needs triage Planning workflow - pending design/dev review. labels Dec 21, 2021
@benelan benelan changed the title Remove usage of getElementDir in favor of CSS logical properties Remove usage of getElementProp Dec 21, 2021
@driskull
Copy link
Member

driskull commented Dec 21, 2021

Taking the first example (accordion-item)...

Instead of the accordion-item looking up at the parent to see what it's iconType, iconPosition or active state, those things should be set by the accordion. So it seems like logic needs to be moved to the parent.

So it seems in general, the logic just needs to move from the child to the parent component and the getElementProp shouldn't be necessary anymore.

The parent component will likely need to setup a mutation observer to do this as well as watchers for when it needs to modify the children.

@driskull
Copy link
Member

@benelan @jcfranco how do we want to divide this up? What sprint?

@benelan benelan removed the needs triage Planning workflow - pending design/dev review. label Dec 30, 2021
@benelan benelan added this to the Back burner milestone Dec 30, 2021
@driskull
Copy link
Member

driskull commented May 4, 2022

This might be something for API consistency epic

@geospatialem geospatialem added 1 - assigned Issues that are assigned to a sprint and a team member. and removed 0 - new New issues that need assignment. labels Jul 19, 2022
@benelan benelan added the Calcite (dev) Issues logged by Calcite developers. label Jan 30, 2023
@geospatialem
Copy link
Member

geospatialem commented Mar 21, 2023

Closing in favor of #6038.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1 - assigned Issues that are assigned to a sprint and a team member. Calcite (dev) Issues logged by Calcite developers. epic Large scale issues to be broken up into sub-issues and tracked via sprints and/or project. refactor Issues tied to code that needs to be significantly reworked.
Projects
None yet
Development

No branches or pull requests

5 participants