Closed tdesvenain closed 13 years ago
This patch worked for us. Would love a merge and then a new release :)
do you plan to merge it ?
I do plan to merge it, but the automatic merge is not going to work here so will need to check the things manually.
Hi, I have put 4545617 into the core. Concerning b11e28f, I had the tests failing and didn't really understand the code. But I have got the idea about the iNavigationRoot. So, I have fixed that properly now and would like to ask you to test whether it works as you intended it to. Please, clone the master for checking. If everything works for you and a couple of other issues, I will roll the release. Thanks for your input
Hi Denis,
I didn't have tested the master for yet, but, you should at least push my tests, shouldn't you ? They obviously match with common usecases.
On Mon, Aug 15, 2011 at 10:02 AM, mishunov < reply@reply.github.com>wrote:
Hi, I have put 4545617 into the core. Concerning b11e28f, I had the tests failing and didn't really understand the code. But I have got the idea about the iNavigationRoot. So, I have fixed that properly now and would like to ask you to test whether it works as you intended it to. Please, clone the master for checking. If everything works for you and a couple of other issues, I will roll the release. Thanks for your input
Reply to this email directly or view it on GitHub:
https://github.com/mishunov/webcouturier.dropdownmenu/pull/3#issuecomment-1805440
Thomas Desvenain
Tlphone : 09 51 37 35 18
On Aug 16, 2011, at 3:02 PM, tdesvenain wrote:
I didn't have tested the master for yet, but, you should at least push my tests, shouldn't you ? They obviously match with common use cases.
They do of course, but there was a problem with them — they fail (at least for me when testing in Plone 4). Plus I have switched from PloneTestCase to plone.app.testing as the main framework for the package. So the tests don't pass and I don't have a lot of time to fix them now. Though I have included the use case you mean with them in the core and there is one test for now. Obviously there should be more tests on this, but unfortunately, as I said, I don't have time to extend the existing ones. If it's critical for you that your tests are included I will do my best to take a look at what is exactly wrong there (if you don't feel like checking them yourself, of course) and why do they fail.
Some things I have noticed — you are using some alien CSS class 'mainTabContent' on the global navigation links, you don't assume spaces in the tags' output when searching in browser.contents, etc. Or a generic thing like 'Root folder appears only once as Home link'. That test doesn't test what you really want to test for. It checks for a simple existence of that string in the output and doesn't care whether it is shown once, twice or hundred times.
Those are straight way to the failure of a test and need to be updated.
Best regards, Denys Mishunov
mailto:denys.mishunov@gmail.com IRC: spliter(#plone)
Ok i understand. i'll have a look on this, and i'll try to improve my tests.
Thank you Denys !
Thomas
On Tue, Aug 16, 2011 at 6:58 PM, mishunov < reply@reply.github.com>wrote:
On Aug 16, 2011, at 3:02 PM, tdesvenain wrote:
I didn't have tested the master for yet, but, you should at least push my tests, shouldn't you ? They obviously match with common use cases.
They do of course, but there was a problem with them they fail (at least for me when testing in Plone 4). Plus I have switched from PloneTestCase to plone.app.testing as the main framework for the package. So the tests don't pass and I don't have a lot of time to fix them now. Though I have included the use case you mean with them in the core and there is one test for now. Obviously there should be more tests on this, but unfortunately, as I said, I don't have time to extend the existing ones. If it's critical for you that your tests are included I will do my best to take a look at what is exactly wrong there (if you don't feel like checking them yourself, of course) and why do they fail.
Some things I have noticed you are using some alien CSS class 'mainTabContent' on the global navigation links, you don't assume spaces in the tags' output when searching in browser.contents, etc. Or a generic thing like 'Root folder appears only once as Home link'. That test doesn't test what you really want to test for. It checks for a simple existence of that string in the output and doesn't care whether it is shown once, twice or hundred times.
Those are straight way to the failure of a test and need to be updated.
Best regards, Denys Mishunov
mailto:denys.mishunov@gmail.com IRC: spliter(#plone)
Reply to this email directly or view it on GitHub:
https://github.com/mishunov/webcouturier.dropdownmenu/pull/3#issuecomment-1818197
Thomas Desvenain
Tlphone : 09 51 37 35 18
Great, i have switched to master on the project, and i don't get any problem, included those my pull requests were about.
Thank you !
Thomas
On Tue, Aug 16, 2011 at 7:07 PM, thomas desvenain < thomas.desvenain@gmail.com> wrote:
Ok i understand. i'll have a look on this, and i'll try to improve my tests.
Thank you Denys !
Thomas
On Tue, Aug 16, 2011 at 6:58 PM, mishunov < reply@reply.github.com>wrote:
On Aug 16, 2011, at 3:02 PM, tdesvenain wrote:
I didn't have tested the master for yet, but, you should at least push my tests, shouldn't you ? They obviously match with common use cases.
They do of course, but there was a problem with them they fail (at least for me when testing in Plone 4). Plus I have switched from PloneTestCase to plone.app.testing as the main framework for the package. So the tests don't pass and I don't have a lot of time to fix them now. Though I have included the use case you mean with them in the core and there is one test for now. Obviously there should be more tests on this, but unfortunately, as I said, I don't have time to extend the existing ones. If it's critical for you that your tests are included I will do my best to take a look at what is exactly wrong there (if you don't feel like checking them yourself, of course) and why do they fail.
Some things I have noticed you are using some alien CSS class 'mainTabContent' on the global navigation links, you don't assume spaces in the tags' output when searching in browser.contents, etc. Or a generic thing like 'Root folder appears only once as Home link'. That test doesn't test what you really want to test for. It checks for a simple existence of that string in the output and doesn't care whether it is shown once, twice or hundred times.
Those are straight way to the failure of a test and need to be updated.
Best regards, Denys Mishunov
mailto:denys.mishunov@gmail.com IRC: spliter(#plone)
Reply to this email directly or view it on GitHub:
https://github.com/mishunov/webcouturier.dropdownmenu/pull/3#issuecomment-1818197
Thomas Desvenain
Tlphone : 09 51 37 35 18
Thomas Desvenain
Tlphone : 09 51 37 35 18
Wonderful. Thanks for letting me know.
It's mainly a commit that makes it properly works with subsites There is also a commit that makes it installable in the context of an other theme than default ones