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

change rule dialog to be conform new wireframes #137

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

change rule dialog to be conform new wireframes #137

wants to merge 89 commits into from

Conversation

bartvde
Copy link
Contributor

@bartvde bartvde commented Aug 1, 2012

@ahocevar
Copy link
Contributor

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
Copy link
Contributor Author

bartvde commented May 30, 2012

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
Copy link
Contributor

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

@ahocevar
Copy link
Contributor

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
Copy link
Contributor Author

bartvde commented Aug 1, 2012

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
Copy link
Contributor Author

bartvde commented Aug 1, 2012

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

@bartvde
Copy link
Contributor Author

bartvde commented Aug 2, 2012

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

@bartvde
Copy link
Contributor Author

bartvde commented Aug 2, 2012

@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
Copy link
Contributor Author

bartvde commented Aug 2, 2012

hey @ahocevar the four issues you raised have been addressed

Bart van den Eijnden added 2 commits August 2, 2012 16:26
@ahocevar
Copy link
Contributor

ahocevar commented Aug 3, 2012

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants