civicrm / org.civicrm.shoreditch

Other
45 stars 59 forks source link

Another 'white button issue' #512

Open eileenmcnaughton opened 3 years ago

eileenmcnaughton commented 3 years ago

See the 'columns' section here

image

But if I deselect the theme setting there is a hidden button

image

@reneolivo I think you just pushed a release but I don't think this is covered?

@colemanw any thoughts

reneolivo commented 3 years ago

Hello @eileenmcnaughton. Will have a look in the morning.

In the mean time, do you have more details about this one? Was this a regression? Has it always been like this? Was this introduced after updating to the latest CiviCRM version?

Thanks a lot for reporting it.

colemanw commented 3 years ago

To clarify, screenshots are from editing a Display within Search Kit.

The button is inside a fieldset's <legend> element, in case that detail matters.

eileenmcnaughton commented 3 years ago

@reneolivo the button is new in the 5.37 rc so not a regression but hopefully easily fixed

eileenmcnaughton commented 3 years ago

@reneolivo so to replicate on the 5.37 tarball - you need to 1) enable the search kit extension 2) go to search->search-kit and create a search 3) add a display to the search - the button appears in the display window - here is a bigger image of the screen

image

deb1990 commented 3 years ago

@eileenmcnaughton @colemanw This is actually expected behaviour, because the class used in core for the Add button is btn-default-outline and that class has white border and white font color.

2021-08-30 at 1 30 PM

So in this case we need to use btn-secondary-outline, which will look like

2021-08-30 at 1 30 PM

I am happy to create a core pr for this.

deb1990 commented 3 years ago

PR opened at https://github.com/civicrm/civicrm-core/pull/21315. cc @eileenmcnaughton @colemanw .

colemanw commented 3 years ago

Thanks. I've merged that PR into core, but I still think the Shoreditch style is problematic - with the current color choice in Shoreditch the button is completely invisible on a white background, and very hard to read on a grey background as in your screenshot.

2021-08-30 at 1 30 PM

deb1990 commented 3 years ago

@colemanw In bootstrap 3, the concept of outline buttons are not present. This was added in Shoreditch. Now, in bootstrap 4 or 5, outline buttons are present, and the main color of the button type is used as the border and font color for the outlined version. For example, if Primary button has blue background, Primary Outline button will have blue border and font color. So similarly the buttons are designed in Shoreditch. As the btn-default has White background, the outline version also has white font and border.

But I understand, that in our case this is problematic, because the default outline version is not visible because of white background. And people might expect the default version to work seamlessly.

So as I see it we have three options,

  1. Keep it as it is, and it is the devs job to never use btn-default-outline on a white background.
  2. We make the btn-default-outline with dark gray font and border color.
  3. We remove the btn-default-outline completely, so that this confusion is avoided. And as this class never exists in original bootstrap, it should not be a big deal.

My opinion is that option 2 should not be done, as it violates the way other outline buttons are defined. So either we go for option 1 or 3. I am personally okay with both of them.

Thoughts? Also @swastikpareek @reneolivo please let me know your opinions too.

swastikpareek commented 3 years ago

@deb1990 : I think that -3 "We remove the btn-default-outline completely" is not the right way to go forward, because if we remove btn-default-outline then other button outline classes needs to be removed too in order to keep things consistent. If we just remove btn-default-outline we creates inconsistency in button outline classes. In future it could create a confusion that why only the default variant button class is not present when other variant outline button classes works fine. I see the main argument behind this option/approach is the readability issue. The white border and thin text doesn't have much contrast with grey background. I would suggest to lighten the page background grey shade in order to improve the contrast rather than removing the default button class altogether. Another argument for not removing the button-default-outline class is that this is being used at other places and removing it may create styling issues on those pages.

For option -1. I don't think it should be Devs responsibility to not to add the btn-default-outline class on buttons inside the white background component. As the business layer (extension's HTML) should be independent on the styling layer and this only has issue with shoreditch theme. So, no matter what classes are added on white background color (provided it should follow HTML semantics) the theme should be responsible (in our case shoreditch) for making sure that it handles the issue by itself. So, in this case I think the best approach would be to handle this scenario and explicitly changing the border/text color of btn-default-outline if it is inside the white background color component.

jamienovick commented 3 years ago

@colemanw @eileenmcnaughton

I think the real problem here is that we have a somewhat incomplete visual design language for Shoreditch with an unfinished styleguide. As such there are likely a) issues with the current implementation (i.e. default buttons are white that are not legible in panels which should be white) but also b) a lack of clarity over how different elements should be used to create a cohesive UI (i.e. should things be in panels, or not at all as the style guide has a blue background and isn't completely in a panel / nor provides guidance on this). I think there was some naivety from the team that worked on this originally that probably could do with some review and improvement to form into a more complete visual language with better clarity across the board...

deb1990 commented 3 years ago

Agreed @jamienovick . So instead of doing to quick fix for the Default Outline button we will review the designs once more to find out similar cases and also to build an examples/kitchensink page. So for the time being lets use btn-secondary-outline class, and we will get back to this soon.