Designer/Engineer - Visibility Component Bug

[CheetoMao]

The Visibility component strips other classNames when wrapped around a single child element.

example:

• .rev-Brand Link is wrapped in the Visibility component
• In the browser, the .rev-Brand class is removed and visibility className is added to the Link

When the Visibility component wraps 2 children, as span is generated with the visibility className that wraps the 2 or more elements.

I think I'd rather have the Visibility component work this way... but it needs some thought as wrapping multiple elements can mess up flex positioning. Also all of the 'showFor[size]' classes are using display:block to show the hidden element. This would have to be overwritten if the element needed a different display type (flex, table, inline-block, ect..)

Questions:

• should we remove the showFor[size] classes in favor of only hiding elements? This would eliminate any display issues since the options would be display: none while the hideFor[size] class was enabled and default to whatever display type was set for the element when the hideFor[size] class was not applied.

• should the visibility class always generate a span that wraps it's children element(s)? Issues with this approach are breaks is flex positioning by introducing a new wrapper. A fix for this would be wrapping each element individually so that the parent/child structure is maintained... is that too much of a pain in the arse?

• one could also use the visibility classNames without using the visibility component, but this often times will not overwrite the display styles given to an element. Should we include !importants for these classNames since they are utility-only? note: I've already added !important to the .hide--[size]Only visibility classes. Didn't add it to the 'show' classes since that would make it even more difficult to overwrite if dispaly: block wasn't the display type you wanted.

[CheetoMao]

I assigned all designers to this ticket in case you have thoughts/opinions on how this is handled. Feel free to unassign yourself if you don't care of have already stated your opinion.

[brittanygay]

Looking into this now and will provide some thoughts/opinions

[brittanygay]

1. I like the idea of removing the showFor[size] classes, as long as we have every hideFor[size] className instance set up that we may need

2. I don't like the idea of generating a <span> that wraps the visibility child elements, since this will break any instances where we may be using flex, and it just sounds messy and extra time-consuming to fix.

I've tried pulling this issue up to play around with in the inspector (on the live site), but it looks like this issue may have been resolved? The rev-Brand className isn't removed when the visibility component is added (but maybe we removed the Visibility component around this instance for now and used classNames instead)?

3. Shouldn't all utility classNames in the utils stylesheets always overwrite the styles given to an element? I think @blazebarsamian and I came across this issue on TSS, where we found ourselves writing !importants for utility classes because they were being overwritten/not applied first. Maybe it's a compiling issue that we need to look into? And possibly an easy fix.

I can look further into these issues, and may be missing something, but this is my initial feedback after giving it a little thought. We could also break out #3 in a new ticket if we need to!

[jwietelmann]

Upon reflection... Should visibility even be a component? Or should we start thinking about utility class functions as first-class citizens? Something like this?

<MyComponent className={`${showFor('medium', 'large')} someOtherClass`} />

[CheetoMao]

@jwietelmann I like the idea of utility classes. My only gripe about a show-for-[size] utility class is that it would add display: block to the css and we can't be certain that the block value wouldn't crap up the styles for an element that may need display: flex/inline/ ect.

propose we use hide-for-[size] utility classes instead, since display: none will definitely hide the element and removing the hide-for-[size] class will allow the default display value to be used.

[jwietelmann]

I like that idea. Anyway, the function itself is pretty simple, so here it is if you want to do this:

function hideFor(...sizes) {
  return sizes.map(size => `hide-for-${size}`).join(' ')
}

[revelry-stalebot[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.