planetfederal / gxp

High level components for GeoExt based applications.
http://boundlessgeo.com/
Other
84 stars 97 forks source link

change rule dialog to be conform new wireframes #137

Open bartvde opened 12 years ago

bartvde commented 12 years ago

See 3.1 in https://img.skitch.com/20111207-emxhsty7ai4uxqrqrjqst42619.png

ahocevar commented 12 years ago

Hey @bartvde, thanks for your work on this. I didn't get to looking into it until today, and now the pull request does not merge correctly any more. Would you mind taking a look at it - maybe you can update the pull request with less effort than me trying to resolve the conflicts. Let me know if you don't get to it. Thanks!

bartvde commented 12 years ago

hey @ahocevar I've updated the pull request. Btw, none of the things that you, @scrollie and @dwins discussed in NY have been incorporated yet.

ahocevar commented 12 years ago

As discussed with @bartvde, this should not be merged before http://product.opengeo.org/browse/SUITE-380 is addressed.

bartvde commented 12 years ago

This is now ready for review again. UI changes have been implemented, so e.g.: no buttons needed, automatically add new node when another node is checked, drag and drop support in the tree.

ahocevar commented 12 years ago

Thank you so much @bartvde for digging through this. I'm already focussed on the OpenLayers sprint, but will review this when I'm back

ahocevar commented 12 years ago

This is great work @bartvde, but as with all big changes, there are still some issues. To make it easier to reproduce the issues, I added the styler plugin to the viewer.html and viewer-layermanager.html (the LayerProperties dialog does not provide styling any more). I also removed the Ext.ux code from the repository, so we can have it added by build tools or on the application level. To make the pull request show my branch, I did the following:

curl -d '{"issue":"137","head":"ahocevar:rule","base":"master"' -u 'ahocevar' https://api.github.com/repos/opengeo/gxp/pulls

You should be able to use this to switch it back to your branch when you make changes.

Now for the issues - there may be more, but these are the ones I found from quickly playing with the new rule dialog, invoked from either viewer.html or viewer-layermanager.html when editing the styles of the usa:states layer:

  1. The Save button is unnecessary. Changes are persisted as soon as they are made. @scrollie, should the button just be removed? Should the Cancel button be renamed to make it obvious that it actually means something like "Revert"?
  2. The layout of the Symbology tab does not play well with scrolling. See here for an example.
  3. The layout of the tree grid is broken. Look at the alignment of the Preview column with the preview swatches here.
  4. The list of sub-symbolizers in the tree grid on the Symbology tab does not match the actual style. The preview on the Rule tab is correct though. For the usa:states layer, the ">4M" rule e.g. shows a black outline as polygon stroke, but the rule does not have a symbolizer with an outline. This can also be seen here.
bartvde commented 12 years ago

hey @ahocevar thanks for your review. I've created a rule branch in the opengeo gxp, which should avoid us having to switch all the time in this pull request. I'll have a look at the issues you found now.

bartvde commented 12 years ago

wrt 2, somehow the colGroup's col elements are not getting a width, will investigate a bit more.

bartvde commented 12 years ago

okay the reason for #2 is apparently that the grid is not getting a fixed width in Rule.js

bartvde commented 12 years ago

@ahocevar but wrt #1 nothing really changed right with this pull request? Before changes were also saved on the fly on the 'change' event. But I agree that removing Save and changing Cancel into Revert would make more sense.

bartvde commented 12 years ago

hey @ahocevar the four issues you raised have been addressed

ahocevar commented 12 years ago

Thanks @bartvde for addressing these issues so quickly. I'll do more testing as soon as I get to it.