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

Allow extension of getAttributes for Tab and TabSet #9954

Merged
merged 2 commits into from Oct 18, 2021

Conversation

lekoala
Copy link
Contributor

@lekoala lekoala commented May 27, 2021

because getAttributes are overriden, extensions cannot update the tabs attributes

this is an issue, and more specifically because injector is not used to create tabset (therefore, extending the class doesn't help)

i don't see any negative effects of this. the alternative is to use injector (TabSet::create instead of new TabSet) in the code to allow for custom classes.

Copy link
Contributor

@maxime-rainville maxime-rainville left a comment

Choose a reason for hiding this comment

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

I'm not sure about this. It seems a bit weird that we are overriding a parent method without calling the parent method. Thought, that wouldn't solve your problem any way, because the parent hook would be called before the Tab/TabSet attributes got added.

Either way, I think there would be value in updating the spots where the Tab/TabSet are instantiated to use the injector instead of new

@kinglozzer
Copy link
Member

I'm not sure about this. It seems a bit weird that we are overriding a parent method without calling the parent method. Thought, that wouldn't solve your problem any way, because the parent hook would be called before the Tab/TabSet attributes got added.

That’s the same with all other form fields too. Perhaps we could adjust them all to behave something like this?

public function getAttributes()
{
    $this->beforeExtending('updateAttributes', function(&$attributes) {
        $attributes = array_merge(
            $this->attributes,
            [
                'id' => $this->ID(),
                'class' => 'tab ' . $this->extraClass()
            ]
        );
    });

    return parent::getAttributes();
}

@lekoala
Copy link
Contributor Author

lekoala commented May 28, 2021

Just maybe to give a bit of context, this is what I ran into. I created a FormFieldExtension and I was checking the class owner. Obviously I expected the tab class to call the updateAttributes extension point but there was nothing. I was confused and checked why : it's because it's overridden in the tab class. This is not very intuitive because you see the extension point in the base class that is later removed in the child class (in my opinion, extensions points should not be removed as they are part of the "class contract").
My goal was to add a new class to the tab (i wanted a regular tab-pane class for bootstrap 5 instead of tab). Meanwhile I managed to do that in the template, so there are ways around it, but still, I find this very odd to remove the extension point. I thought that maybe the reason is because the tab is not a "real" field and maybe it makes sense to split the dataless fields from regular fields. Or maybe have a method specific for tab (updateTabsAttributes?).

Any solution works for me, and maybe there are other classes that have the same issue so it's maybe better to deal with this globally and not just change how tabs are dealt with.

Copy link
Contributor

@maxime-rainville maxime-rainville left a comment

Choose a reason for hiding this comment

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

Having a second look at it now, while the PR's approach is not the perfectly elegant solution of my dreams, it's still an improvement over the status quo ... and there's not an obviously better solution.

@maxime-rainville maxime-rainville merged commit 6e2955f into silverstripe:4 Oct 18, 2021
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.

None yet

3 participants