Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate Query Snippets to React #3627

Merged
merged 21 commits into from
Jul 9, 2019
Merged

Migrate Query Snippets to React #3627

merged 21 commits into from
Jul 9, 2019

Conversation

gabrieldutra
Copy link
Member

@gabrieldutra gabrieldutra commented Mar 23, 2019

What type of PR is this? (check all applicable)

  • Refactor

Description

Migration for the Query Snippets Pages.

WIP:

  • Migrate the List Page
  • Migrate Creation/Edit Dialog
  • Review UX and cleanup
    • treat routes /query_snippets/new and /query_snippets/:id?

Related Tickets & Documents

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

list-query-snippets

@gabrieldutra gabrieldutra self-assigned this Mar 23, 2019
@ghost ghost added the in progress label Mar 23, 2019
@ranbena
Copy link
Contributor

ranbena commented Mar 23, 2019

Taking a step back here to figure out the best ux for this feature.
When I look through my gists (imo, comparable to snippets) what's important to me is:
a) The title
b) The content
c) Sorted chronology.

Although a table view is not ideal for this type of content, I think it would be best to:

  1. Bring back snippet content for list to be scannable. Perhaps format it with <code/> + max-width (300px?) + max-height (~10 lines and webkit clamping).
  2. Remove "updated at" (who cares?)
  3. Remove time from "created at" (leave only date).
  4. Bonus - side menu "My snippets" / "All snippets" (and in the future "Favorites")

@arikfr
Copy link
Member

arikfr commented Mar 24, 2019

When I look through my gists (imo, comparable to snippets) what's important to me is:

You use Gists to share small pieces of code with others. You create snippets to share & use them when composing queries.

Taking a step back here to figure out the best ux for this feature.

If we really want to figure out the best UX for this feature, we should start by moving it to the correct location: the query editor (#2645). As we won't do this right now, let's focus on migrating it to React and keeping as is for now until we prioritize the work to improve this feature.

@ranbena
Copy link
Contributor

ranbena commented Mar 24, 2019

keeping as is for now

What @gabrieldutra suggested though does not keep as is - some content removed, some added.

@gabrieldutra
Copy link
Member Author

Thanks @ranbena, that was the exact kind of feedback I wanted 😁. My goal here for the list page was to migrate it in the easiest path (keeping it as a table list was the case), but with small improvements.

I didn't notice #2645 (should've searched related discussions first) 😅, but a simple table view will do it for now. As for the Create/Edit Dialog, I'm sure it can be used in the future.

  1. Bring back snippet content for list to be scannable. Perhaps format it with <code/> + max-width (300px?) + max-height (~10 lines and webkit clamping).

<code> was a nice suggestion, but imo with a table view it didn't look very good with a lot of lines, I left it with 3 in max and 500px of max-width. I also reduced the number of items per page from 20 to 10. LMK your opinion for the page after this.

  1. Remove "updated at" (who cares?)

👍

  1. Remove time from "created at" (leave only date).

👍

  1. Bonus - side menu "My snippets" / "All snippets" (and in the future "Favorites")

I don't think the API has support for this now. We'd have to work with JS to do it. It's actually not that hard, but I think the best for this one will be to leave it 😐

list2

@gabrieldutra gabrieldutra changed the title Migrate Query Snippets Pages to React Migrate Query Snippets List to React Mar 28, 2019
@gabrieldutra gabrieldutra marked this pull request as ready for review March 28, 2019 23:09
@gabrieldutra
Copy link
Member Author

I decided to change this PR to be only for the List Pages. I did some tests with the Creation/Edit Dialog Form and I want to discuss the way to develop the form before moving to it (and as the List page is already done...)

@gabrieldutra gabrieldutra changed the title Migrate Query Snippets List to React Migrate Query Snippets to React Apr 3, 2019
@gabrieldutra gabrieldutra changed the title Migrate Query Snippets to React WIP: Migrate Query Snippets to React Apr 3, 2019
arikfr
arikfr previously requested changes Apr 4, 2019
Copy link
Member

@arikfr arikfr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two things about the create snippet view:

  1. The text area should have more lines by default (3-4?).
  2. It should update the URL (to query_snippets/new) and of course open the new snippet dialog when using this URL.
  3. Direct links to snippets should work as well (example).

@gabrieldutra
Copy link
Member Author

Two things about the create snippet view:

Done

@arikfr, @ranbena following the discussion about Dialogs, as I mentioned, for this one I opted to use a Dialog because it seemed more future-proof. To improve a bit experience in this case I limited the textarea from 3 ~ 6 rows. This way the form doesn't get too long and it has some flexibility. I imagine the great majority of snippets are not that big, so this should not be an issue. This is how it looks:

query-snippets-dialog

LMK if you have a different idea about it 🙂

@gabrieldutra gabrieldutra changed the title WIP: Migrate Query Snippets to React Migrate Query Snippets to React Apr 7, 2019
@gabrieldutra gabrieldutra requested a review from arikfr April 7, 2019 23:09
@gabrieldutra gabrieldutra dismissed arikfr’s stale review April 29, 2019 18:54

requested changes were made

@gabrieldutra gabrieldutra added Frontend: React Frontend codebase migration to React and removed in progress labels May 7, 2019
Copy link
Member

@arikfr arikfr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last thing: when opening a snippet or the new snippet dialog, the URL should update.

@gabrieldutra
Copy link
Member Author

One last thing: when opening a snippet or the new snippet dialog, the URL should update.

Updated 👍

I actually choose to just change QuerySnippetsList to use the links, so if it's used somewhere else in the future (e.g: Query page) the dialog won't change the URL. Any functional reason for this in the Query Snippets or it's more to create the "changing context" idea?

BTW, all the others create dialogs don't have this behavior.

@gabrieldutra gabrieldutra requested a review from arikfr May 14, 2019 22:36
@arikfr
Copy link
Member

arikfr commented May 15, 2019

I actually choose to just change QuerySnippetsList to use the links

The problem here is that it noticeably refreshes the list, which feels like flickering when the request is fast and might introduce some lag when it's not.

so if it's used somewhere else in the future (e.g: Query page) the dialog won't change the URL.

You can encapsulate the URL changing logic in the showSnippetDialog function, right?

Any functional reason for this in the Query Snippets or it's more to create the "changing context" idea?

The goal is to allow the user to share what they see with others and to be able to refresh the page and to get back to the same place.

$route.current = lastRoute;
un();
});
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arikfr and @kravets-levko, this was the logic I had to introduce to allow changing the URL without reloading. Technically when you update the path without reloading you're not actually navigating anywhere, but this was the method used to change angular route on React. LMK if you think it's better to create a new method and move this or rename it.

BTW, I thought about using a JS method so we'd not depend on Angular:

// change url without reloading
if (window.history.pushState) {
  window.history.pushState({}, '', url);
}

The problem is that Angular didn't work well with it (a lot of $digest's in sequence). Anyway, this will be replaced when we move to another router.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😖

@kravets-levko any thoughts or we can merge this?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, I thought about using a JS method so we'd not depend on Angular:

It's possible, but it may break some AngularJS internals ($location may become out of sync with url). Also it may be a similar problem when we'll move to some React router.

@arikfr I think we can merge this. @gabrieldutra maybe just add a short comment which describes that patch and note that we should keep this in mind when replacing Angular's router with new one.

Copy link
Member Author

@gabrieldutra gabrieldutra Jun 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kravets-levko I've just added a comment with the well known ANGULAR_REMOVE_ME note 👍.

@gabrieldutra gabrieldutra merged commit de0a44e into master Jul 9, 2019
@gabrieldutra gabrieldutra deleted the react-query-snippets branch July 9, 2019 12:29
The-Alchemist pushed a commit to The-Alchemist/redash that referenced this pull request Jul 15, 2019
harveyrendell pushed a commit to pushpay/redash that referenced this pull request Nov 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Frontend: React Frontend codebase migration to React
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants