On Thu, Apr 14, 2016 at 7:40 PM, Dave Page <[email protected]> wrote:
> 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. > Thanks for committing the patch. I'll work on the above comments. Meanwhile I have found one issue where "View Filtered Row" is not working, so attached is the patch file to fix that. Can you please review/commit it. > > -- > Dave Page > VP, Chief Architect, Tools & Installers > EnterpriseDB: http://www.enterprisedb.com > The Enterprise PostgreSQL Company > > Blog: http://pgsnake.blogspot.com > Twitter: @pgsnake > -- *Akshay Joshi* *Principal Software Engineer * *Phone: +91 20-3058-9517Mobile: +91 976-788-8246*
Fixed_View_Filtered_Row_issue.patch
Description: Binary data
-- Sent via pgadmin-hackers mailing list ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgadmin-hackers
