Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.5-RC1
    • Component/s: None
    • Labels:
      None

      Description

      currently the order of css contributions is

      page (1st)>container(2nd)>component(3rd)

      we should inverse this order and allow deepest nested components to contribute first, this way an outer container's contributions will appear below allowing the container to override the css/javascript

        Activity

        Igor Vaynberg created issue -
        Hide
        Igor Vaynberg added a comment -

        put this up for vote before starting work, its a big change

        Show
        Igor Vaynberg added a comment - put this up for vote before starting work, its a big change
        Igor Vaynberg made changes -
        Field Original Value New Value
        Fix Version/s 1.5-M1 [ 12313078 ]
        Hide
        Juergen Donnerstag added a comment -

        I've designed a pluggable header render strategy. The default implementation is parent first, which is what we have today and I implemented a child first one. They are configurable via IApplicationSettings. I've also added a test case to validate the generated output. I built it on 1.5 but as far as I can tell, it does not change any existing API and thus could be applied to 1.4 as well.

        Show
        Juergen Donnerstag added a comment - I've designed a pluggable header render strategy. The default implementation is parent first, which is what we have today and I implemented a child first one. They are configurable via IApplicationSettings. I've also added a test case to validate the generated output. I built it on 1.5 but as far as I can tell, it does not change any existing API and thus could be applied to 1.4 as well.
        Hide
        Juergen Donnerstag added a comment -

        With the pluggable approach you can even mix them. Lets says parent first for wicket core and child first for your own components.

        I'll commit that change to 1.5 since it improves the flexibility anyway. And if we want to change the default implementation, we start the vote.

        Show
        Juergen Donnerstag added a comment - With the pluggable approach you can even mix them. Lets says parent first for wicket core and child first for your own components. I'll commit that change to 1.5 since it improves the flexibility anyway. And if we want to change the default implementation, we start the vote.
        Hide
        Igor Vaynberg added a comment -

        juergen,

        something like this cant be pluggable because if i design my custom component against one render strategy, most likely it wont work when used in an app with another one. i think this should be a contract.

        Show
        Igor Vaynberg added a comment - juergen, something like this cant be pluggable because if i design my custom component against one render strategy, most likely it wont work when used in an app with another one. i think this should be a contract.
        Hide
        Juergen Donnerstag added a comment -

        you are right that different strategies per component make no sense, but lets separate the topics a bit:

        • currently the execution logic is burried insight and almost copied & pasted in two places. I extracted that logic into a separate class (header render strategy) which implements a simple interface (one method). IMO in any case a good idea since it improves code quality
        • Than I extended IAppliationSettings to be able to change the implementation (made it pluggable on application level). If we for sure know there is no reason for any app to provide a derivation of that implementation, than we don't need that or we make it final until someone has a proper use case

        The reason why you thought about changing the execution order was probably because you faced that problem. But can we be sure that the new order will not have different but equally valid problems? Wouldn't it be nice if a user had better control over the placement of his header contribution. One option could be something like a scope or prio per contribution which is considered by the header render strategy. If that is too much but we still want some flexibility, than a modified (pluggable) header render strategy would do as well. E.g. render the header of component XYZ always last. That would be simple to implement for any user.

        Show
        Juergen Donnerstag added a comment - you are right that different strategies per component make no sense, but lets separate the topics a bit: currently the execution logic is burried insight and almost copied & pasted in two places. I extracted that logic into a separate class (header render strategy) which implements a simple interface (one method). IMO in any case a good idea since it improves code quality Than I extended IAppliationSettings to be able to change the implementation (made it pluggable on application level). If we for sure know there is no reason for any app to provide a derivation of that implementation, than we don't need that or we make it final until someone has a proper use case The reason why you thought about changing the execution order was probably because you faced that problem. But can we be sure that the new order will not have different but equally valid problems? Wouldn't it be nice if a user had better control over the placement of his header contribution. One option could be something like a scope or prio per contribution which is considered by the header render strategy. If that is too much but we still want some flexibility, than a modified (pluggable) header render strategy would do as well. E.g. render the header of component XYZ always last. That would be simple to implement for any user.
        Hide
        Igor Vaynberg added a comment -

        i agree that having this in a separated part of code is a great idea; what i do not agree with is making it configurable. wicket should provide a single and more importantly consistent runtime behavior for components. this is what allows the components to be reusable. once we allow the users to start tweaking this behavior we will lose reusability of components across projects.

        if you think there should be a consistent way to declare a header contribution to always show up last, then we should make that part of functionality we offer. maybe <wicket:link priority="100">.... or some other means, but it should be a single consistent way of doing it.

        the reason i opened this issue is that the current order is backwards. i also set this as a fix to 1.5 because changing this order in 1.4 will potentially cause havoc on a large percentage of existing code. making this configurable will cause the same amount of havoc from project to project.

        the reason the current order is backwards is that the container should be able to customize the component. this is because the container encapsulates the component.

        so imagine a simple situation where a component contributes component.css. the container wants to customize some of the css the component contributes, so it contributes container.css. in order for the customization to work container.css has to show up after component.css. if we do not provide this functionality then the auther of component has to give users a hook into the method that contributes component.css so the user of the component can override it, and if author does not then the user is SOL.

        Show
        Igor Vaynberg added a comment - i agree that having this in a separated part of code is a great idea; what i do not agree with is making it configurable. wicket should provide a single and more importantly consistent runtime behavior for components. this is what allows the components to be reusable. once we allow the users to start tweaking this behavior we will lose reusability of components across projects. if you think there should be a consistent way to declare a header contribution to always show up last, then we should make that part of functionality we offer. maybe <wicket:link priority="100">.... or some other means, but it should be a single consistent way of doing it. the reason i opened this issue is that the current order is backwards. i also set this as a fix to 1.5 because changing this order in 1.4 will potentially cause havoc on a large percentage of existing code. making this configurable will cause the same amount of havoc from project to project. the reason the current order is backwards is that the container should be able to customize the component. this is because the container encapsulates the component. so imagine a simple situation where a component contributes component.css. the container wants to customize some of the css the component contributes, so it contributes container.css. in order for the customization to work container.css has to show up after component.css. if we do not provide this functionality then the auther of component has to give users a hook into the method that contributes component.css so the user of the component can override it, and if author does not then the user is SOL.
        Igor Vaynberg made changes -
        Fix Version/s 1.5-M2 [ 12315237 ]
        Fix Version/s 1.5-M1 [ 12313078 ]
        Hide
        Peter Ertl added a comment -

        Think of javascript: if the template page provides jquery.js you definitely want it to be rendered first in line before the child page adds jquery.plugin.js and hits its unresolved dependency jquery.

        Show
        Peter Ertl added a comment - Think of javascript: if the template page provides jquery.js you definitely want it to be rendered first in line before the child page adds jquery.plugin.js and hits its unresolved dependency jquery.
        Igor Vaynberg made changes -
        Fix Version/s 1.5-M3 [ 12315329 ]
        Fix Version/s 1.5-M2 [ 12315237 ]
        Jeremy Thomerson made changes -
        Fix Version/s 1.5-M4 [ 12315483 ]
        Fix Version/s 1.5-M3 [ 12315329 ]
        Hide
        Juergen Donnerstag added a comment -

        committed an update to 1.5 which fixes some issues (e.g. tests) with the deepest first render strategy. If we want to enable it for 1.5 we only need to change one line in AbstractHeaderRenderStrategy.get(), and update some test results. And somebody should validate it really works as expected

        Show
        Juergen Donnerstag added a comment - committed an update to 1.5 which fixes some issues (e.g. tests) with the deepest first render strategy. If we want to enable it for 1.5 we only need to change one line in AbstractHeaderRenderStrategy.get(), and update some test results. And somebody should validate it really works as expected
        Hide
        Igor Vaynberg added a comment -

        @Peter: if component wants to contribute jquery.plugin.js it should also contribute jquery.js. components are supposed to be self-contained. and, wicket will filter multiple jquery.js contributions. i think this is a better default, and if we need to add priorities, we can later.

        @Juergen: can you go ahead and enable it and tweak the tests? thanks.

        Show
        Igor Vaynberg added a comment - @Peter: if component wants to contribute jquery.plugin.js it should also contribute jquery.js. components are supposed to be self-contained. and, wicket will filter multiple jquery.js contributions. i think this is a better default, and if we need to add priorities, we can later. @Juergen: can you go ahead and enable it and tweak the tests? thanks.
        Igor Vaynberg made changes -
        Assignee Igor Vaynberg [ ivaynberg ] Juergen Donnerstag [ jdonnerstag ]
        Juergen Donnerstag made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]

          People

          • Assignee:
            Juergen Donnerstag
            Reporter:
            Igor Vaynberg
          • Votes:
            1 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development