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

Another 'white button issue' #512

Open
eileenmcnaughton opened this issue Apr 13, 2021 · 11 comments
Open

Another 'white button issue' #512

eileenmcnaughton opened this issue Apr 13, 2021 · 11 comments

Comments

@eileenmcnaughton
Copy link
Contributor

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
Copy link
Collaborator

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
Copy link
Member

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
Copy link
Contributor Author

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

@eileenmcnaughton
Copy link
Contributor Author

eileenmcnaughton commented Apr 14, 2021

@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
Copy link
Contributor

deb1990 commented Aug 30, 2021

@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
Copy link
Contributor

deb1990 commented Aug 30, 2021

PR opened at civicrm/civicrm-core#21315. cc @eileenmcnaughton @colemanw .

@colemanw
Copy link
Member

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
Copy link
Contributor

deb1990 commented Aug 31, 2021

@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
Copy link
Collaborator

@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
Copy link

@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
Copy link
Contributor

deb1990 commented Sep 1, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants