Re: [patch][pgAdmin] RM7018 [React] Port Restore dialog to React.

Hi Aditya/Akshay,

Please find the comments below.

On Wed, Dec 22, 2021 at 1:57 PM Aditya Toshniwal <
aditya(dot)toshniwal(at)enterprisedb(dot)com> wrote:

> Hi Rahul,
>
> Everything looks good except:
>
> 1. Remove unnecessary whitespaces
>
>
> adityatoshniwal(at)LAPTOP381PNIN pgadmin4_copy % git apply
> ~/Downloads/RM7018.patch
>
> /Users/adityatoshniwal/Downloads/RM7018.patch:14070: trailing whitespace.
>
> constructor(getRestoreSectionSchema, getRestoreTypeObjSchema,
> getRestoreSaveOptSchema,
>
> /Users/adityatoshniwal/Downloads/RM7018.patch:15563: trailing whitespace.
>
> import RestoreSchema, {getRestoreSaveOptSchema,
> getRestoreQueryOptionSchema, getRestoreDisableOptionSchema,
>
> /Users/adityatoshniwal/Downloads/RM7018.patch:15620: new blank line at EOF.
>
> +
>
> warning: 3 lines add whitespace errors.
>
Done

>
> 2. Use lodash, and not underscore.
>
> + 'sources/gettext', 'sources/url_for', 'underscore', 'pgadmin.browser',
> 'sources/utils',
>
It's removed now.

> 3. Use gettext
> panel.title(`Restore (${pgBrowser.Nodes[data._type].label}:
> ${data.label})`);
>
It's fixed now.

> 4. Dialog size is huge. Reduce the dialog size.
>
As per discussion on the call, I have reduced the height of the dialog
(both for restore & backup)

> 5. Mouseover on header should change cursor to "move"
>
It's fixed now.

>
> [image: image.png]
>
> 6. If I remove the bin path from preferences, the "utility not found"
> error comes after I click on restore. It should come as soon as I hit the
> menu. Same problem with backup.
>
Fixed for both restore & backup utility.

> 7. In restore.js the below code is repeated. You should get the treeInfo
> only once the dialog opens.
> const selectedNode = pgBrowser.tree.selected();
> var selectedTreeNode = pgBrowser.tree.findNodeByDomElement(selectedNode);
> const treeInfo = pgBrowser.tree.getTreeNodeHierarchy(selectedTreeNode);
>
Refactored the code, it's removed now.

> 8. Minor spacing fixes between arrow and text.
> ()=>getRestoreQueryOptionSchema({nodeInfo: treeNodeInfo}),
> ()=> getRestoreDisableOptionSchema({nodeInfo: treeNodeInfo}),
> ()=> getRestoreMiscellaneousSchema({nodeInfo: treeNodeInfo}),
>
Have taken care of this.

>
> On Wed, Dec 22, 2021 at 11:35 AM Akshay Joshi <
> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>
>> Hi Aditya
>>
>> Can you please review it?
>>
>> On Tue, Dec 21, 2021 at 4:52 PM Rahul Shirsat <
>> rahul(dot)shirsat(at)enterprisedb(dot)com> wrote:
>>
>>> Hi Hackers,
>>>
>>> Please find the attached patch, which ports the restore dialog to react.
>>>
>>> --
>>> *Rahul Shirsat*
>>> Senior Software Engineer | EnterpriseDB Corporation.
>>>
>>
>>
>> --
>> *Thanks & Regards*
>> *Akshay Joshi*
>> *pgAdmin Hacker | Principal Software Architect*
>> *EDB Postgres <http://edbpostgres.com>*
>>
>> *Mobile: +91 976-788-8246*
>>
>
>
> --
> Thanks,
> Aditya Toshniwal
> pgAdmin Hacker | Software Architect | *edbpostgres.com*
> <http://edbpostgres.com>
> "Don't Complain about Heat, Plant a TREE"
>

--
*Rahul Shirsat*
Senior Software Engineer | EnterpriseDB Corporation.