yext / answers-search-ui

Answers Javascript API Library for building Search experiences.
Other
22 stars 7 forks source link

Change icons from divs to spans #1857

Closed likimmy closed 8 months ago

likimmy commented 8 months ago

To address the first issue in this techops, the icons for answers-search-ui should be wrapped in spans, not divs.

coveralls commented 8 months ago

Coverage Status

coverage: 62.02%. remained the same when pulling 0f6f0bb9929516e996cb4f2999832657c8154cec on dev/make-icons-spans into ff87d8bf7b58f01a1feb0c60e4804a80b01f2b6d on hotfix/v1.16.6.

benmcginnis commented 8 months ago

just to double check, does all the styling in the test site and acceptance test pages look the same after this change? divs and spans are sometimes styled differently

also, it's probably not a big deal, but we may want to call this change out in the release documentation because any changes to the DOM like this could technically break the styling of an existing implementation if they specifically targeted divs that we're changing to spans

The Icon class adds a display: inline-block; to the element it's added to so this shouldn't change anything.

benmcginnis commented 8 months ago

How do we do the callout in the release documentation, Nidhi?

nmanu1 commented 8 months ago

How do we do the callout in the release documentation, Nidhi?

On our side, this would just mean adding an extra note in our GH release notes explaining possible ramifications of the change. But, it would be up to Baigel if he wants it to be documented more widely, such as in a HH post. The likelihood of this breaking someone's styling is small, but technically possible, so I'm not sure how cautious we want to be about it.

As it is right now, if we release this change in a hotfix, all users on v1.16 will get the change automatically without any action on their part (since most implementations pin to a minor version rather than a patch). This could technically mean that styling may suddenly change for them with no action on their part. If we're worried about this possibility, it may be better to release this in a new minor version so implementations wouldn't get the change without deliberately upgrading. If not, then it's probably fine as is

likimmy commented 8 months ago

for this techops, would we need to address all instances of divs being used within a button anywhere in answers-search-ui and the HH theme? or did they specifically only care about answers-search-ui icons?

I'm honestly not sure, I was going off of a code link that was included in the techops that was from answers-search-ui. I'm assuming they only care about the icons from that sdk since they didn't link any other relevant code.

benmcginnis commented 8 months ago

for this techops, would we need to address all instances of divs being used within a button anywhere in answers-search-ui and the HH theme? or did they specifically only care about answers-search-ui icons?

I'm honestly not sure, I was going off of a code link that was included in the techops that was from answers-search-ui. I'm assuming they only care about the icons from that sdk since they didn't link any other relevant code.

I think what's called out in the techops is specifically riven by the the icon component

likimmy commented 8 months ago

to-do item address voiceSearchIcon here

nmanu1 commented 8 months ago

to-do item address voiceSearchIcon here

can we update this item to also include looking into any other usages of a div within a button? if this is not valid html, then we'll want to address all of them