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

Inheritted group combo params #1838

Open
wants to merge 2 commits into
base: dev-3.x
Choose a base branch
from

Conversation

andrewnicols
Copy link
Contributor

This is based on the fix for #1837

…ng is specified

This fixes a regression caused by yui#1832.
@yahoocla
Copy link

CLA is valid!

@andrewnicols
Copy link
Contributor Author

I've added tests for combine, and comboSep, but I wasn't sure how best to test that maxURLLength is applied differently within groups.

comboBase = group.comboBase;
comboSep = group.comboSep;
maxURLLength = group.maxURLLength;
if (typeof group.comboBase === 'string') {
Copy link
Member

Choose a reason for hiding this comment

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

Do we care about comboBase: ''?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why wouldn't we? It should be possible to either specify a comboBase for a group, or to use the inheritted one. There are reasons you may want to do so, largely developer related admittedly.

Copy link
Member

Choose a reason for hiding this comment

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

I don't have a personal stance in this case. But should we stick to backwards compatibility ('' being falsy)?

Copy link
Member

Choose a reason for hiding this comment

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

guys, let's stick to the previous behavior, there is not need to change that behavior now.

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

Successfully merging this pull request may close these issues.

4 participants