Migrate Embed Query Dialog to React #3783

gabrieldutra · 2019-05-09T15:00:12Z

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

Refactor

Description

It was a pretty simple migration, so it seemed a good opportunity to explore Antd's Typography.

To make Typography's <code> better fit the current in Redash we can remove the gray background.

Related Tickets & Documents

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

Alert when embedding query containing text parameters

gabrieldutra · 2019-05-09T15:02:21Z

client/app/components/queries/EmbedQueryDialog.jsx

+    visualization.id}?api_key=${query.api_key}&${query.getParameters().toUrlParams()}`;
+
+  let snapshotUrl;
+  if (window.snapshotUrlBuilder) {


I'm not sure what this is for, but I just followed what was in the Angular version.

kravets-levko · 2019-05-09T15:36:44Z

client/app/components/queries/EmbedQueryDialog.jsx

+      <h5 className="m-t-0">IFrame Embed</h5>
+      <div className="m-b-10">
+        <Text data-test="EmbedIframe" copyable code>
+          {`<iframe src="${embedUrl}" width="720" height="391"></iframe>`}


@gabrieldutra I think it would be better to replace this with <textarea> - so user will be able to copy it manually and it will not require ellipsis (I personally use this dialog just to get embed URL when I need to test something, so it's a case for having all the code available).

I've considered using a TextArea too, but I thought the copy button could be a good feature. I'll check it with the TextArea 👍

I agree with @kravets-levko that we shouldn't use an ellipsis here, but why a textarea?

ranbena · 2019-05-09T17:00:33Z

Wdyt of the following design?

Code block.
Copy button appears in corner on hover.
Allow setting iframe width / height.

gabrieldutra · 2019-05-09T18:56:58Z

Wdyt of the following design?

Seems nice, perhaps without the Checkbox (not really necessary to make width and height disabled since it doesn't really change the behavior) or if you think it has a reason to be there, use a switch instead?

ranbena · 2019-05-09T19:08:12Z

Seems nice, perhaps without the Checkbox (not really necessary to make width and height disabled since it doesn't really change the behavior) or if you think it has a reason to be there, use a switch instead?

I agree, it's not necessary - it serves as a hint for "normal" users that the current setting is default and doesn't require adjustment - "ignorable" if you will. Not a must.

(A switch I think would be too distracting tho)

arikfr · 2019-05-09T19:12:19Z

My suggestions to address some common use cases with this dialog:

Show a copy-able URL for the visualization (the one we use for the iframe). 99% of the times I used this dialog, I just needed to copy this URL :) We can call this "Public URL".
As the next section, show the iframe.
If the query has unsafe parameters (query.is_safe = true), then show a message saying you can't currently embed queries with parameters of type text. This is instead the embed URLs.

gabrieldutra · 2019-05-12T06:29:43Z

I've updated this based on your comments :), just need some final UI adjustments. @ranbena welcome to give your magic look to it already 😆

My thoughts: now that it has two sections the default look for Form Labels (Width and Height) may be confusing, so it's better with a smaller font-weight (I think I'll add an <hr /> to separate sections too). Regarding the checkbox, after playing with the dialog I think it's better with it.

Those are my ideas, I'll update this tomorrow (technically just later today), but you can already give your comments.

arikfr · 2019-05-12T11:07:39Z

Last version looks good 👌 One thing I would change: have the copy button always visible instead of only on hover.

Also, can you add a screenshot of the dialog when you try to embed with unsafe params?

ranbena · 2019-05-12T11:17:30Z

ranbena · 2019-05-12T11:43:24Z

I think it looks good as well 👌👌

Here's an updated version:

It's got:

persistent copy buttons - give the codebox ~~padding-left: 27px~~ padding-right: 27px (:laughing:) so button won't overlap code.
checkbox - has the effect of indenting the row which makes it clearer that it's iframe related
normal weighted labels
small input-numbers + width 72px

ranbena

Looks great!

Just one request @gabrieldutra - change code block to border-radius: 2px (similar to Ant components) 🙏

gabrieldutra

Styling Update
Current:

Alternatively with <hr> (it's sort of cool to separate stuff):

I've updated the dialog's title: "Embed Code" -> "Embed Query", looks more related to what it is

Embed query spec

Now that it has a Public URL separate, no need for a regex 🙂.
With the new alert for unsafe parameters, the flow for getting the URL needs to be done differently (e.g: creating a query with safe parameters, saving the url and then changing it to have unsafe parameters). The simplest was to just remove the snapshot and moving the assertion to the Dialog. This generates a case uncovered from tests, so LMK if there's value on creating another test for the case I mentioned in the example;

CC: @rauchy

gabrieldutra · 2019-05-14T17:59:12Z

client/cypress/integration/embed/share_embed_spec.js

@@ -5,7 +5,7 @@ describe('Embedded Queries', () => {
    cy.login();
  });

-  it('are shared without parameters', () => {
+  it('can be shared without parameters', () => {


Seemed a better naming to me.

client/app/components/CodeBlock.less

ranbena · 2019-05-14T20:09:58Z

Alternatively with <hr> (it's sort of cool to separate stuff)

With the right spacing it could be nice. <Divider dashed /> is also an option.

I personally though would instead go with m-b-30 for the top code block. Looks nice and clean.

<div class="m-b-30">
  <div class="code-block">
    …
  </div>
</div>

I've updated the dialog's title: "Embed Code" -> "Embed Query", looks more related to what it is

👍

arikfr · 2019-05-15T05:38:51Z

👌

* Update Antd * Migrate Embed Query Dialog to React * Update jest ScheduleDialog snapshot * Add Alert for unsafe queries * Add CodeBlock * Add inputs to change iframe size * Undo ant update * Update share embed spec * Update styling * Change border-radius to 2px * Update margin between Public URL and IFrame Embed

gabrieldutra added 2 commits May 9, 2019 11:22

Update Antd

2796904

Migrate Embed Query Dialog to React

ce9451f

gabrieldutra commented May 9, 2019

View reviewed changes

Update jest ScheduleDialog snapshot

0f0827c

gabrieldutra self-assigned this May 9, 2019

kravets-levko reviewed May 9, 2019

View reviewed changes

gabrieldutra added 4 commits May 10, 2019 11:02

Add Alert for unsafe queries

3ef3f6a

Add CodeBlock

598a390

Add inputs to change iframe size

d6a2976

Undo ant update

46b30df

weekly-digest bot mentioned this pull request May 13, 2019

Weekly Digest (6 May, 2019 - 13 May, 2019) #3792

Closed

gabrieldutra added 3 commits May 14, 2019 13:48

Update share embed spec

46c00c7

Merge branch 'master' into react-embed-query-dialog

81c164a

Update styling

8f7e084

ranbena approved these changes May 14, 2019

View reviewed changes

gabrieldutra commented May 14, 2019

View reviewed changes

client/app/components/CodeBlock.less Outdated Show resolved Hide resolved

Change border-radius to 2px

bfced08

Update margin between Public URL and IFrame Embed

57f9100

arikfr merged commit 4f40237 into master May 15, 2019

arikfr deleted the react-embed-query-dialog branch May 15, 2019 05:38

weekly-digest bot mentioned this pull request May 20, 2019

Weekly Digest (13 May, 2019 - 20 May, 2019) #3814

Closed

This was referenced May 13, 2022

[Snyk] Fix for 6 vulnerabilities change/redash#27

Closed

[Snyk] Fix for 2 vulnerabilities change/redash#28

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate Embed Query Dialog to React #3783

Migrate Embed Query Dialog to React #3783

gabrieldutra commented May 9, 2019 •

edited

Loading

gabrieldutra May 9, 2019

kravets-levko May 9, 2019

gabrieldutra May 9, 2019

arikfr May 9, 2019

ranbena commented May 9, 2019

gabrieldutra commented May 9, 2019

ranbena commented May 9, 2019 •

edited

Loading

arikfr commented May 9, 2019

gabrieldutra commented May 12, 2019 •

edited

Loading

arikfr commented May 12, 2019

ranbena commented May 12, 2019

ranbena commented May 12, 2019 •

edited by gabrieldutra

Loading

ranbena left a comment

gabrieldutra left a comment

gabrieldutra May 14, 2019

ranbena commented May 14, 2019

arikfr commented May 15, 2019

Migrate Embed Query Dialog to React #3783

Migrate Embed Query Dialog to React #3783

Conversation

gabrieldutra commented May 9, 2019 • edited Loading

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

Description

Related Tickets & Documents

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

gabrieldutra May 9, 2019

Choose a reason for hiding this comment

kravets-levko May 9, 2019

Choose a reason for hiding this comment

gabrieldutra May 9, 2019

Choose a reason for hiding this comment

arikfr May 9, 2019

Choose a reason for hiding this comment

ranbena commented May 9, 2019

gabrieldutra commented May 9, 2019

ranbena commented May 9, 2019 • edited Loading

arikfr commented May 9, 2019

gabrieldutra commented May 12, 2019 • edited Loading

arikfr commented May 12, 2019

ranbena commented May 12, 2019

ranbena commented May 12, 2019 • edited by gabrieldutra Loading

ranbena left a comment

Choose a reason for hiding this comment

gabrieldutra left a comment

Choose a reason for hiding this comment

gabrieldutra May 14, 2019

Choose a reason for hiding this comment

ranbena commented May 14, 2019

arikfr commented May 15, 2019

gabrieldutra commented May 9, 2019 •

edited

Loading

ranbena commented May 9, 2019 •

edited

Loading

gabrieldutra commented May 12, 2019 •

edited

Loading

ranbena commented May 12, 2019 •

edited by gabrieldutra

Loading