Re: Custom layers in PgAdmin IV Mapview

Поиск
Список
Период
Сортировка
От Dave Page
Тема Re: Custom layers in PgAdmin IV Mapview
Дата
Msg-id CA+OCxoxoUWhR17dRJ8Ny733U7S_hyOK8KcGWZMAQP-+KqZqc4w@mail.gmail.com
обсуждение исходный текст
Ответ на Re: Custom layers in PgAdmin IV Mapview  (Atle Frenvik Sveen <atle@frenviksveen.net>)
Ответы Re: Custom layers in PgAdmin IV Mapview
Список pgadmin-hackers
Hi

On Tue, Sep 18, 2018 at 11:09 AM, Atle Frenvik Sveen <atle@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

В списке pgadmin-hackers по дате отправления:

Предыдущее
От: Dave Page
Дата:
Сообщение: Re: [pgAdmin4][Patch]: RM #3464 pgAdmin 4 won't start on windows
Следующее
От: Atle Frenvik Sveen
Дата:
Сообщение: Re: Custom layers in PgAdmin IV Mapview