Hi
On Tue, Sep 18, 2018 at 6:58 PM, Atle Frenvik Sveen <a...@frenviksveen.net>
wrote:
>
>
> On Tue, Sep 18, 2018, at 17:39, Dave Page wrote:
> > 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
> >
> Oh. I honestly have no idea what happened here, but I'll see what I can
> do.
>
>
> >
> > >
> > > 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.
>
> These buttons are fontawesome, but I guess there are multiple styles?
>
Well, it's more the background/layout style I guess. All the action buttons
(up/down/delete) should be grouped together in the same column with the
same background colour. For consistency of layout I guess we may need to
always show all buttons, but disable up for the top row and down for the
bottom row.
>
>
> > - 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).
> >
> Got you. I moved it to the left, as the right hand of the table is hidden
> behind a horizontal scrollbar on smaller window-sizes. But the app is
> probably meant to be used in full size? Also had some issues with using the
> style used for the "top header" in the other tables. But I'll give it
> another try!
>
>
> > >
> > > 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.
> >
> Great!
>
>
> > > - 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 guess i mostly glanced at existing code and borrowed parts. Did not feel
> comfortable trying to refractor code I did not completely understood. So I
> guess it's okay. But given my lack of familiarity with the code I'll accept
> a second opinion here.
>
>
>
> > > - 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
> >
>
> Ah, the es-comment makes sense. I guess I've violated the practice of
> declaring all variables at the to of a function,other than that I tried my
> best to align with existing code.
>
>
> > > - 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).
>
> So tests for the Python-code? As long as there are tests for the other
> types I think I can figure that out. If not you'll hear from me ;)
>
Yeah - what we normally refer to as the API tests (as that's what they
primarily exercise). I'd look at storing a setting of the new type, then
retrieving it and ensuring it comes back as expected.
>
>
> > Thanks!
>
> Thanks for the comments!
>
> -a
>
>
> --
> Atle Frenvik Sveen
> a...@frenviksveen.net
> 45278689
> atlefren.net
>
--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake
EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
