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

CIWEMB-376: Fix payment scheme listing table #482

Conversation

ahed-compucorp
Copy link
Contributor

@ahed-compucorp ahed-compucorp commented Jul 19, 2023

Overview

This PR updates the payment scheme listing table to :

  • Re-order the columns to match the spec. file.
  • Display the permission label i.e. capitalize the first letter.
  • Remove the uppercase effect from the table header.
  • Add the ID column.
  • Moved the buttons to a separate component.

Please see the civicrm/org.civicrm.shoreditch#560 PR to update the default table styles.

Before

Screenshot from 2023-07-19 12-52-45

After

Screenshot from 2023-07-26 11-42-29

@ahed-compucorp ahed-compucorp changed the title : Fix payment scheme listing table CIWEMB-376: Fix payment scheme listing table Jul 19, 2023
@jamienovick
Copy link

jamienovick commented Jul 19, 2023

@ahed-compucorp
Looks great, but a question, where did we add the styles? Should they be in Shoreditch (Bootstrap) itself perhaps so that all future search kits get those styles?

Also I noticed the buttons are in the table. Should they be separate as they are probably not part of that component?

Thanks

@ahed-compucorp
Copy link
Contributor Author

ahed-compucorp commented Jul 19, 2023

@jamienovick
After looking at Shoreditch theme, the styles there are specific to certain pages and tables. The general styles that got applied to this table (or any new table created by an extension) doesn't account for the buttons, even the width of each column didn't match the similar table like the one in "Manage ACL Roles" :

Screenshot from 2023-07-19 15-47-44

Should they be in Shoreditch (Bootstrap) itself perhaps so that all future search kits get those styles?

Extensions could benefit from updating the "general styles". I'll work on that.

Should they be separate as they are probably not part of that component?

I've done that after looking at other tables like the above one "Manage ACL Roles" and below another table example:

Screenshot from 2023-07-19 15-45-59

Some tables have a button at the bottom of the table.

@jamienovick
Copy link

jamienovick commented Jul 19, 2023

@ahed-compucorp
Do you think we need to have a call to discuss?

I really don't think we should be creating styles for one individual page, especially those that are bootstrap (this is BS?) and using search kit.

I think the example screens you are using are not search kit screen outputs. For example this screen in the latest CiviCRM is created by search kit and is the sort of thing we are targeting to work the same?

https://dmaster.demo.civicrm.org/civicrm/admin/custom/group?reset=1

Built with:
This search kit:
https://dmaster.demo.civicrm.org/civicrm/admin/search#/edit/4
and
Then embedded in this afform:
https://dmaster.demo.civicrm.org/civicrm/admin/afform#/edit/afsearchAdminCustomFields

If the concern is just around the button placement, I think we should just have those at the top separate as a separate component. They don't need to be styled as part of the table. 2 buttons at the top above the table would be fine.

Ideally if you do the styling at the Shoreditch level then all search kit outputs, including those mentioned above, will pick up your nice new styling and jobs all done for making search kit screens look nice with Shoreditch?

Does that make sense or need a call?

ps.

After looking at Shoreditch theme, the styles there are specific to certain pages and tables. The general styles that got applied to this table (or any new table created by an extension) doesn't account for the buttons, even the width of each column didn't match the similar table like the one in "Manage ACL Roles" :

Shoreditch has 2 themes, one for legacy CiviCRM markup and one for bootstrap. See here: https://github.com/compucorp/org.civicrm.shoreditch/tree/85a77a34f1777f5cc6a4e82791d4e4dbed5503ae#using-bootstrapcss

My understanding is the appropriate styles are loaded depending on the type of core screen.

@ahed-compucorp
Copy link
Contributor Author

@jamienovick

Do you think we need to have a call to discuss?

Let's have a call.

I think the example screens you are using are not search kit screen outputs

Yes, you're right. CiviCRM migrated some of the pages/tables to search kit, I wasn't aware of that.

I really don't think we should be creating styles for one individual page, especially those that are bootstrap (this is BS?) and using search kit.

Got it, I'll try using the search kit to render this table.

Shoreditch has 2 themes....

Thank for the info.

@ahed-compucorp ahed-compucorp force-pushed the CIWEMB-376-fix-payment-scheme-listing-table branch from 38c11a4 to 112776a Compare July 26, 2023 08:44
@ahed-compucorp
Copy link
Contributor Author

Got it, I'll try using the search kit to render this table.

I looked into this, but I think it is not clear how to featurize the search-kit-related configurations.

@ahed-compucorp ahed-compucorp merged commit 1bc871c into CIWEMB-84-workstream Jul 26, 2023
@ahed-compucorp ahed-compucorp deleted the CIWEMB-376-fix-payment-scheme-listing-table branch July 26, 2023 11:30
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

Successfully merging this pull request may close these issues.

3 participants