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

FormBuilder - Now you can make Tab Sets 🎉 #31944

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

colemanw
Copy link
Member

@colemanw colemanw commented Feb 4, 2025

Overview

Build tabsets with the FormBuilder drag-n-drop editor!

image

Technical Details

This breaks with the old crmUiTabSet jQuery-Ang-hybrid directive and re-implements tabs with pure AngularJs.

Same functionality, but a bit more clear what's going on
Copy link

civibot bot commented Feb 4, 2025

🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷

Introduction for new contributors...
  • If this is your first PR, an admin will greenlight automated testing with the command ok to test or add to whitelist.
  • A series of tests will automatically run. You can see the results at the bottom of this page (if there are any problems, it will include a link to see what went wrong).
  • A demo site will be built where anyone can try out a version of CiviCRM that includes your changes.
  • If this process needs to be repeated, an admin will issue the command test this please to rerun tests and build a new demo site.
  • Before this PR can be merged, it needs to be reviewed. Please keep in mind that reviewers are volunteers, and their response time can vary from a few hours to a few weeks depending on their availability and their knowledge of this particular part of CiviCRM.
  • A great way to speed up this process is to "trade reviews" with someone - find an open PR that you feel able to review, and leave a comment like "I'm reviewing this now, could you please review mine?" (include a link to yours). You don't have to wait for a response to get started (and you don't have to stop at one!) the more you review, the faster this process goes for everyone 😄
  • To ensure that you are credited properly in the final release notes, please add yourself to contributor-key.yml
  • For more information about contributing, see CONTRIBUTING.md.
Quick links for reviewers...

➡️ Online demo of this PR 🔗

@civibot civibot bot added the master label Feb 4, 2025
@colemanw colemanw changed the title Tabset2 FormBuilder - Now you can make Tab Sets 🎉 Feb 4, 2025
@mattwire
Copy link
Contributor

mattwire commented Feb 4, 2025

@nicol @artfulrobot is this good from a theming point of view?

@colemanw
Copy link
Member Author

colemanw commented Feb 4, 2025

@nicol @artfulrobot is this good from a theming point of view?

I think there are 2 lines of css in this PR to port to riverLea. Also significant changes to the markup of the "Campaign Dashboard" screen, but it's all BS3-compliant markup.

/* Tabs */
#bootstrap-theme .af-tabpanel {
display: block;
padding: 10px;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is probably ok, but flagging to @vingle as generally like to avoid px but I think poss for this use it's ok. Or there may be a --var that's better?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for flagging, I didn't get this – as confusingly I'm 'nicol' on gitlab and vingle here.

@artfulrobot - all that matters for now is RiverLea adds this line, and swaps it for rems, then (var(--crm-m2) = 10px). Too many pixels in core to sweat.

If this is a normally padding instance for something with a border, then --crm-padding-reg (16px) or --crm-padding-inset (8px) might be a better variable as it lets Streams can adjust all instances at once.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's nothing precise about the 10px here, it just gives a bit of padding so the tab content doesn't look squashed against the edge. Feel free to use a value more consistent with other styles.

@vingle
Copy link
Contributor

vingle commented Feb 5, 2025

Have had a quick test of the Tab Building UI - works nice and intuitively and seems to work with RiverLea well (prob thanks to the BS3 markup).

That said when I try to view my test tabbed form (http://core-31944-7exld.test-1.civicrm.org:8003/civicrm/quick-add/Individual) - it doesn't seem to display - not sure if that's a config shortfall on my side or an actual bug.

@colemanw
Copy link
Member Author

colemanw commented Feb 5, 2025

@vingle I think the only problem is you gave your form the same url as another form. I changed it to something else and now it works: http://core-31944-7exld.test-1.civicrm.org:8003/civicrm/quick-test

Allows a tabset element to be configured in FormBuilder
This was a challenge! I had to rework the afTabset & afTab components,
because there's no good way to access
properties of a controller in transcluded html.

Using transclude and then accessing `$parent.count` worked, but I'm not sure why.
Angular scope is dark magic...
@colemanw colemanw added the merge ready PR will be merged after a few days if there are no objections label Feb 5, 2025
@vingle
Copy link
Contributor

vingle commented Feb 5, 2025

@colemanw - ah thanks.

The tabs themselves work nicely but it doesn't pick up the wrapper. Is there any reason you didn't re-use BS3's panel wrapper with a tab pane using panel-body? As that's the pattern in API4 explorer and FB editor - so it would then inherit the rest of the theme.

@vingle
Copy link
Contributor

vingle commented Feb 5, 2025

(tho, tbf, there's a bunch of different tab markup classes used in different bits of civi - ie searchkit listings use a slightly different pattern)

@colemanw
Copy link
Member Author

colemanw commented Feb 5, 2025

@vingle The tabs themselves work nicely but it doesn't pick up the wrapper. Is there any reason you didn't re-use BS3's panel wrapper with a tab pane using panel-body? As that's the pattern in API4 explorer and FB editor - so it would then inherit the rest of the theme.

I think BS3 panels and tabs are different widgets. As you note, some screens in Civi combine them to get a tabbed panel, but they can also be used separately.

@vingle
Copy link
Contributor

vingle commented Feb 5, 2025 via email

@colemanw
Copy link
Member Author

colemanw commented Feb 5, 2025

@vingle I guess I didn't see this as inventing a new pattern but rather re-using the BS3 standard. FormBuilder tends to follow BS3 markup conventions.

@vingle
Copy link
Contributor

vingle commented Feb 5, 2025

@colemanw the tab buttons themselves use BS3, that's why they look like tabs in Greenwich (and RiverLea):

image

But the wrapper around the tabs has no class and the tab pane has a new class .af-tabpanel. If you don't want to use .panel & .panel-body as some bits of Civi use to define tab regions, could we add a new class, ie crm-tabs and crm-tabs-pane? It would I think have a low impact to add this to the markup, but at least then there would be clear classes to target to put, say, a border/drop-shadow around the entire tab block, and padding/bg colour on the tab pane itself..

FWIW, I appreciate even BS5 doesn't create tab wrappers (https://getbootstrap.com/docs/5.1/components/navs-tabs/) yet in Civi all the tab instances have a bounding border, there isn't the pure nav tab style that Bootstrap references, it's always in a box.

image

When it comes to listings of SearchKit and FormBuilder the lack of this bounding wrapper class creates a bit of a headache to target for consistent themeing…

Eg. to create the border around the tab pane below I'm doing some really specific targeting, that then has to be repeated with different specific targets for the FormBuilder listing.. it would save a lot of effort if there was just a consistent crm-tab type class on the wrapper.

image

NB: not trying to delay a really good PR, just piggybacking it to try limit a problem that's not been well articulated by me, from becoming a bigger problem.

@ufundo
Copy link
Contributor

ufundo commented Feb 5, 2025

Bit of a curveball, but saw something about adding a new tab implementation.

If doing that and want it to be the tab implementation, then it would be cool to consider leapfrogging AngularJS and using a WebComponent. I've been experimenting and seems to work pretty seamlessly across AngularJS / Smarty templates and has the beauty of being 50 lines of JS with no dependencies...

Can try a PR tomorrow..

@colemanw
Copy link
Member Author

colemanw commented Feb 5, 2025

@ufundo sounds good to me. Implementing the tabs in ang was only a few lines of code so not much to port.

@vingle
Copy link
Contributor

vingle commented Feb 6, 2025

@ufundo that sounds good. A while ago I tried to find a JS-free tab approach, and the only ones are hacky involving checkboxes.

A native-JS alternative would be a big improvement as it's not tied into BS3.js/Jquery. Main requirement is it's keyboard controllable, and uses something like ul/li for the nav so it's easy to make responsive.

Then I'd say my wish for better class naming on this PR isn't important, as it can be improved with Ufundo's proposal at a later point, and it's currently possible to target the three key elements:

af-tabset {
  border: 1px solid #ccc;
}
.af-tablist {
  background-color: #ddd;
}
.af-tabpanel {
  background-color: #eee
}

(in case this was holding up a merge)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master merge ready PR will be merged after a few days if there are no objections
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants