Disabled 'Running...' button while query running #106

jgellin-sf · 2020-12-16T19:12:21Z

What does this PR do?

This PR provides the SOQL Builder UI-side function for indication that a query is running. When the Run Query button is pressed, the button is transformed into a disabled Running... button, and remains so until VSCode sends the run_query_done message.

What issues does this PR fix or reference?

@W-8207738@

codecov · 2020-12-16T19:12:30Z

Codecov Report

Merging #106 (8cfa39f) into develop (65eb027) will increase coverage by 0.48%.
The diff coverage is 95.28%.

@@             Coverage Diff             @@
##           develop     #106      +/-   ##
===========================================
+ Coverage    94.16%   94.64%   +0.48%     
===========================================
  Files           42       43       +1     
  Lines         1079     1288     +209     
  Branches       228      274      +46     
===========================================
+ Hits          1016     1219     +203     
- Misses          61       67       +6     
  Partials         2        2

Flag	Coverage Δ
language-server	`96.93% <97.17%> (-3.07%)`	⬇️
soql-builder-ui	`92.71% <100.00%> (+0.08%)`	⬆️
soql-model	`95.14% <92.10%> (+0.15%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...s/querybuilder/services/message/soqlEditorEvent.ts	`100.00% <ø> (ø)`
...kages/soql-model/src/serialization/deserializer.ts	`93.04% <92.10%> (+0.45%)`	⬆️
packages/language-server/src/completion.ts	`96.79% <96.79%> (ø)`
packages/language-server/src/validator.ts	`97.50% <100.00%> (-2.50%)`	⬇️
...oql-builder-ui/src/modules/querybuilder/app/app.ts	`86.73% <100.00%> (+0.41%)`	⬆️
...ilder-ui/src/modules/querybuilder/header/header.ts	`100.00% <100.00%> (ø)`
...ui/src/modules/querybuilder/services/toolingSDK.ts	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ec8fbb3...8cfa39f. Read the comment docs.

packages/soql-builder-ui/src/modules/querybuilder/app/app.test.ts

dehru · 2020-12-16T20:11:24Z

packages/soql-builder-ui/src/modules/querybuilder/services/toolingSDK.test.ts

+    postMessageFromVSCode({
+      type: MessageType.RUN_SOQL_QUERY_DONE
+    });
+    expect(runStateObserver.mock.calls.length).toBe(2);


Is this 2 because behavior subject gets an immediate callback with the current value?

dehru · 2020-12-16T20:20:04Z

packages/soql-builder-ui/src/modules/querybuilder/services/message/standaloneMessageService.ts

  private localStorage = getLocalStorage();
  postMessage(messageObj) {
    this.window.parent.postMessage(messageObj, '*');
+    if (messageObj.type === MessageType.RUN_SOQL_QUERY) {


we actually do the full round trip across an iframe using this development repo that behaves like vscode. So you can just recieve the message there and respond using a timeout.

https://github.com/forcedotcom/soql-tooling-standalone/blob/main/src/client/modules/my/app/app.ts#L23

Ah cool. I've moved it to the tool in this https://github.com/forcedotcom/soql-tooling-standalone/pull/3 and removed from this PR with the Move standalone run query response to standalone tool commit.

dehru

This UX has bugged me since the start. Great to get this in. I had one comment about the setTimeout to mock the response. Let's move that to the standalone server repo. So you can literally just remove that whole mocking of the response from this PR.

jonnyhork

Looks good for the most part, nice and simple. Just a few comments on the CSS and how we want to approach JS/CSS overall in the UI.

jonnyhork · 2020-12-16T20:53:26Z

packages/soql-builder-ui/src/modules/querybuilder/cssCommon/cssCommon.css

+button:disabled {
+  color: var(--vscode-foreground, var(--soql-foreground));
+  background-color: var(
+    --vscode-list-inactiveSelectionBackground,
+    var(--soql-color-light-grey)
+  );
+  cursor: default;
+}


I have also created a btn--disabled class for the where component which is "hidden" in the feature/where branch, hopefully we won't miss things too much when working off develop.

I would like to keep some consistency throughout the extension if we can. So I think we should have one CSS class for disabled buttons in the CSS Common here that used for both features. I'm not sure what the best way to approach that is between the branches.

I took a slightly different approach to disabling the button and did not use a pseudo class.
I also chose a darker grey for more contrast with the white text but that might be theme dependant, etc

I can't find any info on whether its better( or matters) to use the disabled attribute vs using CSS to render the button disabled. Our choice will determine the JS we use to toggle attributes/class though.

For example this method would toggle disabled instead returning the classlist.

@dehru or @jgellin-sf Do you have a preference on what approach we take? Either way I think we should be consistant with how we handle these types of behaviors.

For the sake of JS ease of use, I think it's probably better to handle disabled button just using CSS as you have done rather than with the attribute. Then it's treated as just another class and not this special case.

I agree it shoud be handled consistently in the CSS common. I see in the where branch the CSS def is in where.css. Maybe you can move it to CSS common and we can merge the WHERE stuff, then I'll change to that class when I rebase.

ok, well my branch is already merged to feature/where but I can create a new PR to move the class to CSS Common, is that what you were thinking?

Yeah that sounds good.

@jgellin-sf I created and merged #108 to the feature/where for your pleasure.

jonnyhork · 2020-12-16T21:08:23Z

packages/soql-builder-ui/src/modules/querybuilder/header/header.html

+      <template if:false={isRunning}>
+        <button class="run-button" onclick={handleRunQuery}>Run Query</button>
+      </template>
+      <template if:true={isRunning}>
+        <button class="running-button" disabled>Running...</button>


Another way to do this without two templates is by making the text of the button dynamic like we do with the fields throughout the UI.

return this.isLoading ? 'Loading...' : 'Select Field...';

In this case, if we did not use the templates, the classes and the disabled would require some JS and I think would be more work without that much benefit other than consistency I guess.

I do not see the class running-button in the CSS though are you still using it?
I probably would have used a different class name than run-button if I did this again, I'm trying out this whole BEM thing for new work.
For example:

header__button--run /* or ready, enabled */ header__button--running /* disabled */ header__button-run--enabled header__button-run--disabled

I am still getting used to it but the idea is to be consistant and predictable with our class names.

Actually for BEM naming I don't think we include the header part... but same idea

It turns out I wasn't styling run-button or running-button so I've removed those from the template in the Remove un-styled classes commit.

Whoops acted too fast. run-button is used in the test code. Restored now.

That's relevant as we have discussed moving away ( slowly ) from using css classes as selectors in testing code, because it's often not apparent that the class is for testing vs styling.

I agree with moving that direction

* model WHERE clause (#68) * Remote SOQL Errors - Language Server Part (#93) * remove code builder (#92) * Move from antlr4JS to antlr4TS (#96) * SOQL code completion for SELECT fields (#99) * publish language server 0.2.9 (#102) * Fix LSP dependency (#104) * Disabled 'Running...' button while query running (#106) * Add basic code-completion for ORDER BY and other improvements (#107) * Dehru and JG telemetry - Send GDPR clean data to telemetry when errors/unsupported syntax present (#110) * Disable WHERE in model (#113) * Bump versions for publishing (#114) * Code-completion for WHERE clause expressions (#112) Co-authored-by: Jonathan Gellin <jgellin@salesforce.com> Co-authored-by: jgellin-sf <55159130+jgellin-sf@users.noreply.github.com> Co-authored-by: Chase McCarthy <charles.mccarthy@heroku.com> Co-authored-by: Jonny Hork <jhork@salesforce.com> Co-authored-by: Fernando Dobladez <fernandodobladez@salesforce.com> Co-authored-by: Jonathan Gellin <jgellin@salesforce.com>

Disabled 'Running...' button while query running

188baff

jgellin-sf requested review from amphro, code0100fun, dehru, jonnyhork, mysticflute, praksb and tmock12 as code owners December 16, 2020 19:12

dehru reviewed Dec 16, 2020

View reviewed changes

packages/soql-builder-ui/src/modules/querybuilder/app/app.test.ts Outdated Show resolved Hide resolved

dehru reviewed Dec 16, 2020

View reviewed changes

dehru suggested changes Dec 16, 2020

View reviewed changes

jonnyhork reviewed Dec 16, 2020

View reviewed changes

jgellin-sf added 2 commits December 16, 2020 16:19

remove unused import

eeea895

Move standalone run query response to standalone tool

270ce87

jgellin-sf requested a review from dehru December 16, 2020 21:35

jgellin-sf added 3 commits December 16, 2020 16:39

Remove un-styled classes

eaa4f7b

restore class used in test code

64d5406

use consistent css classes with where branch

8cfa39f

dehru approved these changes Jan 4, 2021

View reviewed changes

dehru merged commit c1b9590 into develop Jan 4, 2021

dehru deleted the jg/runningQueryIndicator branch January 4, 2021 20:54

Disabled 'Running...' button while query running #106

Disabled 'Running...' button while query running #106

Uh oh!

Conversation

jgellin-sf commented Dec 16, 2020

What does this PR do?

What issues does this PR fix or reference?

Uh oh!

codecov bot commented Dec 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dehru left a comment

Choose a reason for hiding this comment

Uh oh!

jonnyhork left a comment

Choose a reason for hiding this comment

Uh oh!

jonnyhork Dec 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Dec 16, 2020 •

edited

Loading

jonnyhork Dec 16, 2020 •

edited

Loading