Hi, See my comments below. On Wed, Jul 6, 2011 at 8:59 PM, Kalle Korhonen <[email protected]>wrote:
> Dragan, the demo site you've put together looks very nice indeed, I > strongly encourage other to take a look as well! Comments below. > > On Wed, Jun 29, 2011 at 2:47 PM, [email protected] > <[email protected]> wrote: > > 1. Dropdown menu (see demo page) > > - As this is the most common scenario (IMO) fro using a contextmenu, I > > tried to provide a dropdown menu that is a separate component meant to be > > used as a context menu. This is still Work in Progress and any feedback > on > > it would be helpfull. > > - The goal with the dropdown menu is to make it's usage as simple as > > possible making 2nd level submenus in tml etc. Please see the source code > > and tml of the demo page (links to the source code are on the demo page > > also). > > Good work, this component (and the underlying context menu) looks > relatively close to being ready for inclusion. It might be a good idea > to focus on making it complete enough to be included as a patch. > There's always room to improve, but it can be enhanced even while it's > already part of Tapestry core. That would get you to work more using > Apache/Tapestry workflow. Before making a patch though, a few specific > issues: > - documentation, especially javadoc > - the default css should be an asset, rather than pulled from the context > - consider using .t-dropdown etc. as CSS style class name instead of > .dropdown and so on - too generic name may collide more easily and > .t-* is inline with tapestry conventions > Noted. Already on it. I'll write an update these days when I finish these tasks, and also I was thinking of writing some integration tests, very similar to the tests for the existing components. - copyrights - have you copied any code/styles from anywhere you > haven't written yourself? > Actually I also wanted to discuss this. I had looked at many css dropdown menus, before doing it and the one I implemented is "inspired" by this menu <http://www.lwis.net/free-css-drop-down-menu/dropdown.simple.linear.html> which is dual licenced <http://www.lwis.net/free-css-drop-down-menu/> (bottom of page) under MIT <http://opensource.org/licenses/mit-license.php> and GPL<http://www.gnu.org/copyleft/gpl.html> (I know its not compatible with apache). But the thing is that Its not the same CSS its still quite changed and I was not sure if I have reused and changed it, or just written something new "inspired" by it :). I avoided doing copy-> paste, I typed the CSS as I saw it best. Anyway, please provide advice about this, that I will gladly accept, because I don't have much experience dealing with licences and I want to avoid making mistakes regarding this. - hideType=mouseOut is always problematic as it depends on where the > new object is rendered. At least the current example didn't quite work > for me (on FF5, it was hidden as soon as it was rendered) - maybe > consider a larger/transparent area around the actual menu as "inside" > Tested In FF5, now I see what you mean. Will take a look at it and try to improve it. - what's the wait for in onContextMenuFromGrid1(...) of the dropdown > example? > I have just copied it from the grid examples. When working locally the ajax is fast so I cannot see the "loading ..." text, that is why I put Thread.sleep. Its just for the demo site it wont be in the tests. > > > 2. Grid Enhancements - Making pagination and Sorting > > Bookmarkable (see demo page) > > - This is achieved by modifying the existing GridColumns and > GridPager (see > > line 141) components with just a few lines of code, combined with mixins > for > > the actual URL manipulation (bookmarking). > > - The main issue why I had to modify GridPager and GridColumns is because > > they don't keep (off course) the URL parameters (request parameters) for > the > > links for the sort columns (in GridColumns) and for the links in the page > > numbers (in Grid Pager). The added LOC are just to keep those parameters > for > > their action and sort events. This approach obviously needs rethinking. > One > > solution I could think of is decorating the links but this happens in > pages > > not components, so I would have to add advice to all pages (tried that > also > > and it worked), but it seems like just too much overhead. > > - Another idea is to just make a redirect on setupRender on the grid's > > mixins to put the parameters in the url. This is the simplest solution > and > > it would also work if the grid is sorted in code (not by clicking the > > columns), but a redirect is another request. IDK if this can lead to > usage > > problems. Advice please :) > > - So. All Work is done in the 2 mixins ColumnsSort.java (for Grid > > Columns) and CurrentPageURL.java (for GridPager) that catch the sort and > > action events and add parameters and also read parameters from the > Request > > to adjust column sorting and pagination. I supose this is quite ok. But > the > > main issue remains. > > "How to keep the request parameters for the links produced by GridPager > and > > GridColumns?" > > Unfortunately, I don't have good suggestions for this at the moment. > If it was possible to advice the component, it sounds like that could > have been a good enough solution but I agree advising all pages is too > heavyweight as a solution. For redirects, while it's reasonable safe > to assume that the datamodel is backed by some cache, we cannot be > sure and so might have an adverse effect on performance. I'll spend a > bit more time on it to see if there are other reasonable alternatives. > Yes, I have tried few (working) solutions but with just too much overhead on advices etc. I'm still searching for the best way to do it, even considering small changes in the GridColumns and GridPager code. When I have something new for this problem I'll post it here. > > > 3. Expose the cell content in GridCell.java > > - Another issue I was advised to look into by my mentor (Kalle) is to > > replace the advice that makes ContextMenu.java work for the tapestry > grid. > > Basically this is the idea: Modify GridCell.java to expose the grid cell > > contents (row value and property value not just property value like in > > PropertyOutputContext.java) in an environmental. This can be used in far > > more scenarios, not just contextmenu for the grid (think of a mixin that > > selects a grid row (or cell) and notifies the server via ajax, so you can > do > > master-detail pages etc). > > Replacing the GridCell advice with a few lines of code is next on my work > > log so we'll se how it goes. > > Yes, this makes sense to me and it's fairly cheap to do. > Kalle > Cheers, Dragan Sahpaski > > > > On Wed, Jun 15, 2011 at 1:27 PM, [email protected] > > <[email protected]> wrote: > >> > >> Hi, > >> I have updated the contextmenu with other client events like mousedown, > >> mouseover etc. that you can look at on the new examples > >> page http://dragansah.com/contextmenu/parametersexamples. > >> You can browse the issues (mainly tasks and bugs) > >> here > http://code.google.com/a/apache-extras.org/p/right-click-menu-gsoc2011/issues/list > and > >> if someone has some ideas or wishes, they can add them to the issues > list. > >> Next on my work-log is to provide a dropdown menu as a separate > component > >> that can be used with the existing contextmenu. It is not directly > related > >> to the contextmenu because you can put anything in the contextmenu, but > >> maybe 90% of the use-cases of a contextmenu is to have a dropdown wiht > icon > >> and a label like they have on the rich faces demo site under > >> RichMenu->ContextMenu. > >> Any kind of comments or remarks are very much welcomed. > >> Cheers, > >> Dragan Sahpaski > >> > >> > >> On Tue, May 31, 2011 at 7:16 PM, Kalle Korhonen > >> <[email protected]> wrote: > >>> > >>> I'm officially Dragan's mentor but the rest of committers should > >>> really take a look at Dragan's excellent work if you can just spare a > >>> few cycles for it. The demo site (http://dragansah.com/contextmenu/) > >>> Dragan put together makes it easy to track the project status even if > >>> you don't have time to dive into the code. We already talked about the > >>> items via Skype but a few quick comments below.. > >>> > >>> On Sun, May 29, 2011 at 1:51 PM, [email protected] > >>> <[email protected]> wrote: > >>> > 1. Two mixins: ContextMenu and ContextMenuAjax or merge them in one > >>> > mixin? > >>> > >>> It doesn't cost any more to use separate mixins for the user so > >>> probably makes sense to keep as is for now. > >>> > >>> > 2. Is the concept of advising GridCell (actualy > >>> > >>> AbstractPropertyOutput< > http://tapestry.apache.org/tapestry5/apidocs/org/apache/tapestry5/corelib/base/AbstractPropertyOutput.html#renderPropertyValue(org.apache.tapestry5.MarkupWriter > , > >>> > java.lang.String)>) ok? Its clear that we cannot implement support > of > >>> > the > >>> > t5 grid without an advice or I'm I wrong? > >>> > >>> That's the way and it didn't seem too bad at all, but if it'd be > >>> possible to implement it better by making a small, generic and > >>> backwards compatible enhancement to GridCell, I think that'd be > >>> preferable in the long run, just because there's some performance > >>> penalty whenever using an advice, and an AbstractPropertyOutput gets > >>> used a lot. > >>> > >>> > 5. The contextmenu is rendered after the component and there is a > bit > >>> > of > >>> > The quirk is that the contextmenu forces the render of the > conainer's > >>> > clientId (if it is a t5 ClientElement), and uses it to set a > >>> > javascript > >>> > event oncontextmenu on it using the id. If the component is not a > t5 > >>> > ClientElement (t5 Label for example), than this javascript is > applied > >>> > for > >>> > getting the client element in js: > $('contextMenuId').previousSibling. > >>> > This > >>> > code assumes that the parent component is only one html element > (div > >>> > or > >>> > similar), but this is not the case for example the t5 TextField > which > >>> > renders a trailing icon. Luckily the TextField is covered because > it > >>> > is a > >>> > >>> Seems to me it's quite fine to require ClientElement in that case. > >>> > >>> Kalle > >>> > >>> --------------------------------------------------------------------- > >>> To unsubscribe, e-mail: [email protected] > >>> For additional commands, e-mail: [email protected] > >>> > >> > > > > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: [email protected] > For additional commands, e-mail: [email protected] > >
