Hi On Thu, Apr 14, 2016 at 1:58 PM, Akshay Joshi <[email protected] > wrote:
> Hi All > > I have fixed review comments given by Dave and couple of them are remaining > > *Fixed Review Comments:* > > - The View Data menu option should be on the Object menu, which should > mirror the Context menu, except options should be disabled when not > applicable instead of hidden. > - The Query Tool menu icon should be a glyphicon, to match the others. > - Please merge the functionality of the Refresh and Execute buttons > into one button. We shouldn't have two buttons that do essentially the same > thing. > - In Edit Grid mode, the History panel should log all queries (SELECTs, > UPDATEs, DELETEs etc) as it would in the Query Tool. > - In Edit Grid mode, the Messages panel should display any messages > from the most recent action as it would in the Query Tool. > - In Edit Grid mode, that textbox should be read-only, but should > display the SQL used (including any LIMIT/FILTER clauses) > - Please remove the border from the SQL box, such that it fills all > available space. > - The Filter box should be in a modal overlay over the top of the SQL > box/Results tabs as required. Those elements should be grayed whilst it is > open. > - Please adjust the height of the Delete icon in the Edit Grid, such > that it doesn't force the row height to be higher than it should be. > - I think the names of the tabs are far too long. Can we change them > to "Query 1", "Query 2" etc, then rename them to the filename if the > user saves/loads a file? > - We should add an "Edit" button, which opens a drop-down menu. This > would eventually include options as found on the Edit menu in the pgAdmin3 > query tool, such as the "Clear SQL" option. > - Ashesh and I discussed changing the History tab to be a grid, > showing: Date/Time, Query (first line only), Rows affected, Runtime > and Status, in a row per query executed. Ashesh suggested using a > sub-form that can be expanded for each row, which could show the full query > and error details (SQL State etc). New rows should be added to the top > of the list. > - Errors should be highlighted in the SQL box - a marker in the margin > to note the line, and spellcheck-style underlining for the error word. > - Query results should have spaces converted to " ", so that > proper indenting is maintained (for example, on EXPLAIN queries). > - on shutdown pgAdmin server , appropriate message should be displayed. > > Awesome! I've made a few minor style changes - mostly colouring, but I also widened the Execute button and it was easier to push it's dropdown than it - and pushed the code. > *Remaining review comments*: > > - Please add an SQL button. This should show/hide the SQL panel in > *both* Query Tool and Edit Grid modes. > - If a field has been edited, but not saved, can we highlight it > somehow? Maybe make the text dark blue? > - The "Add Row" button only works if you're on the last page of the > resultset. > - Can the "Copy Row" button also populate the clipboard with CSV data > for the row? > - In Edit mode, we need to be able to represent/set values to NULL. > - The layout of the result tabs should be maintained if new Query Tool > or Edit Grid tabs are opened. > > *TODO's* (apart from above) > > - Open/Save SQL file. > - Cut, Copy, Paste functionality. > > Agreed on the above. My only real suggestion on the code itself (which looks good and clean on a quick review), is that: - The button bar be moved out into an HTML template - create.sql should perhaps be renamed to insert.sql - It looks like we only allow updates if we have a pkey. Should we allow use of OIDs when present, if a pkey isn't there? I'll continue to log additional issues if/when I find them. -- Dave Page VP, Chief Architect, Tools & Installers EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company Blog: http://pgsnake.blogspot.com Twitter: @pgsnake
