Re: [GSoC] Query History Upgrade

Hi Aditya,

On Mon, Aug 12, 2019 at 9:04 AM Aditya Toshniwal <
[email protected]> wrote:

> Hi Yosry,
>
> Nice work there. It seems to be working fine except a few suggestions:
> 1) Fix pep8 issues
> 2) DOM Statements like below can be avoided and html can be added directly
> to main template of $el instead of adding extra operations of find, prepend
> and append. Plus, it makes it difficult to understand what will the DOM
> look like.
> this.$el.find('.query').prepend('<i id="query_source_icon"
> class="query-history-icon sql-icon-lg"></i>');
> $container.append($toggleEl).append(self.$el);
> 3) Change the below to use class d-none with toggleClass('d-none') for
> consistency across.
>     this.$el.find('.pgadmin-query-history-entry').each(function() {
>       $(this).toggle();
>     });
>

Please find an updated patch attached with the above issues fixed. The pep8
issue was in the test, I didn't re-check pep8 after writing the test - my
bad.


> 4) I may be wrong, but I'm seeing the flash icon for view/edit data
> queries and view table icon for query tool queries. Looks like they are
> swapped.
> [image: Screenshot 2019-08-12 at 12.15.18.png]
>
>
>
They seem to be in the right place for me, would you mind rechecking?

Thanks !

-- 
*Yosry Muhammad Yosry*

Computer Engineering student,
The Faculty of Engineering,
Cairo University (2021).
Class representative of CMP 2021.
https://www.linkedin.com/in/yosrym93/