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

[major] BEM-XJST: modifier templates should apply before def() #490

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

Conversation

miripiruni
Copy link
Contributor

Fixes #482

@miripiruni miripiruni requested a review from qfox October 30, 2017 15:45
@coveralls
Copy link

coveralls commented Oct 30, 2017

Coverage Status

Coverage remained the same at 99.306% when pulling 020fa6a on issue-482__mod-templates into ba102b7 on master.

@miripiruni
Copy link
Contributor Author

Looks like it’s patch… but I still have doubt about it.

@@ -350,7 +350,7 @@ def()(value)

The `def` mode (short for "default") has a special status. It is responsible for generating the result as a whole. This mode defines the list of other modes and the order to go through them, as well as the build procedure for getting the final representation of the HTML element or BEMJSON from the parts generated in the other modes.

This is a special mode that shouldn’t be used unless truly necessary. A user-defined template that redefines `def` disables calls of the other modes by default.
This is a special mode that shouldn’t be used unless truly necessary. A user-defined template that redefines `def` disables calls of the other modes by default, exept `mods`, `elemMods`, `addMods` and `addElemMods`.
Copy link
Member

Choose a reason for hiding this comment

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

except

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zxqfox thanks!

Copy link
Member

@qfox qfox left a comment

Choose a reason for hiding this comment

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

Looks good!

@coveralls
Copy link

coveralls commented Oct 30, 2017

Coverage Status

Coverage remained the same at 99.387% when pulling e542cd7 on issue-482__mod-templates into e76cd62 on master.

@miripiruni
Copy link
Contributor Author

miripiruni commented Jan 16, 2018

I want to check it on real front-end project with gemini tests. @tadatuta @zxqfox @sbmaxx who uses v8.x?

@sbmaxx
Copy link
Contributor

sbmaxx commented Jan 16, 2018

Turbo? ;)

@miripiruni miripiruni force-pushed the issue-482__mod-templates branch 2 times, most recently from 8664edb to 12575ca Compare February 5, 2018 13:00
@miripiruni
Copy link
Contributor Author

@zxqfox it’s a major changes.

Integration tests are red. https://nda.ya.ru/3TwoUQ

Template from turbo:

block('list')(
    def()((node, ctx) => applyNext({ items: [].concat(ctx.content || []) })),

    addMods()((node, ctx) => {
        const mods = { [ctx.ordered ? 'ordered' : 'unordered']: true };

        ctx.ordered && node.items.length > 10 && (mods['wide-numbers'] = true);

        // у ВинФонов в шрифтах нет некоторых символов, используемых для маркеров
        if ((node.data.reqdata.device_detect.OSFamily || '').toLowerCase() === 'windowsphone') {
            mods['alt-markers'] = true;
        }

        return mods;
    }),

    

@miripiruni miripiruni modified the milestones: 8.8.8, 9.0.0 Feb 5, 2018
@qfox
Copy link
Member

qfox commented Feb 5, 2018

Okay, let's consider this as breaking, but it's a bad code on their side, they can't guarantee they have items in node cuz there is no applyNext call.

😢 but true.

@miripiruni
Copy link
Contributor Author

PR is locked and waiting next major release.

@miripiruni miripiruni changed the title BEMXJST: modifier templates should apply before def() [major] BEM-XJST: modifier templates should apply before def() Apr 10, 2018
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.

5 participants