Hi
On Tue, Sep 18, 2018 at 11:09 AM, Atle Frenvik Sveen <a...@frenviksveen.net>
wrote:
> Hello again!
>
> I think I've accomplished this task now, see last screenshot and attached
> patch.
>
> I hope I did this patch right?
> git checkout master
> git fetch upstream
> git merge upstream/master
> git checkout mapviewer_custom_layers_3646
> git merge master
> mapviewer_custom_layers_3646
> git format-patch master --stdout > RM3645.patch
>
Looks right I think - though something got missed as the resulting patch
doesn't actually apply:
dpage@hal:*~/git/pgadmin4*$ git apply ~/Downloads/RM3645.patch
/Users/dpage/Downloads/RM3645.patch:1919: trailing whitespace.
.fa-clickable {
/Users/dpage/Downloads/RM3645.patch:1920: trailing whitespace.
cursor: pointer;
/Users/dpage/Downloads/RM3645.patch:2062: trailing whitespace.
.fa-clickable {
/Users/dpage/Downloads/RM3645.patch:2063: trailing whitespace.
cursor: pointer;
error: patch failed: web/package.json:69
error: web/package.json: patch does not apply
error: patch failed: web/yarn.lock:31
error: web/yarn.lock: patch does not apply
>
> The OrderedList preferences type seems to work fine, and the layout works
> okay by my standards.
> - You are able to edit existing entries (but elements that are marked
> required but not filled in are reverted)
> - You are able to add a new entry (but all required elements must be
> filled before it's saved)
> - Elements can be deleted (I was thinking about disallowing the "default"
> entries, but not sure if this makes sense)
> - Elements can be re-ordered (first in list is default)
>
Couple of fixes we really need for UI consistency:
- The up/down buttons should be FontAwesome icons, following the same
formatting as the delete icon.
- The Add button should be at the right hand end of the (missing) table
header, with just the + icon in it (see the Privileges or Security Labels
tables on the Security tab of the Database properties dialogue for an
example).
>
> The maviewer seems to pick up the preferences just fine, and the default
> preferences are entered as best as I could (mainly replicating the
> hard-coded layers).
>
> Some improvement-points I guess:
> - You can add several new elements without filling them in (though they
> won't be saved)
>
Some of the existing tables allow that, so I don't think that's the end of
the world.
> - No validation of wether the TileUrl is valid (but this is a difficult
> task, not worth it?)
>
I think that's fine - but we should have good documentation in
preferences.rst to help the user get it right.
> - Possibly better explaination (but for me, who knows this, it's difficult
> to improve on)
>
I'll review and tweak any strings as part of the the review/commit. As long
as you put something there that I can understand, I can massage the wording.
> - I was not able to re-use code everywhere I wanted, so some duplication
>
To what degree? If it's entire functions, then we should look at factoring
them out into shared modules somewhere.
> - I was not quite sure about code-style, so feel free to comment on that.
>
Nothing stands out as being wrong. The main thing is to make it look like
the rest of the code, so it doesn't appear out of place. There are also
some basic notes at
https://www.pgadmin.org/docs/pgadmin4/dev/coding_standards.html
> - This seemes pretty hard to write sane tests for, so I skipped that part.
I agree for the actual maps. However, I would like to see tests for the
preference storage/retrieval as you've added a new type. Aditya and
Khushboo can give guidance on how to do that if needed (as well as the
layout).
Thanks!
--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake
EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
