Jhork/custom select from #130

jonnyhork · 2021-01-27T17:20:44Z

What does this PR do?

This PR will add support a single selection mode of the Custom Select component and other improvements. This PR will also replace the standard <select> element with the Custom Select component to search and chose Objects in the SOQL builder.

New Features

@api value : string[] can be used to query state of the component for selections made independently of the model. This will help with the WHERE clause when we need to know that a selection has been made but wait until all modifiers have a value before updating the model.
If there are multiple Custom Selects on the page then only one will be allowed to display its options at a time.
The width of the component will be determined by its parent element up to a max width. NOTE: currently the options menu will still span the full width of the form column.
Added a arrow icon ⌃ that will match standard <select>elements and toggle the options menu open or closed.
The placeholder text will fade out when the user clicks on the input
Added comments for posterity

Single Select Mode

If there is a selected option, then it will be displayed as the value of the component.
When the user clicks on the input the value will be highlighted (selected) so the user can start typing to replace it.

Single.Select.Mode.mov

What issues does this PR fix or reference?

@W-8616118@

…ork/custom-select-from

codecov · 2021-01-27T17:20:51Z

Codecov Report

Merging #130 (273211c) into feature/custom-select (e7e4e68) will increase coverage by 0.14%.
The diff coverage is 94.11%.

@@                    Coverage Diff                    @@
##           feature/custom-select     #130      +/-   ##
=========================================================
+ Coverage                  87.96%   88.11%   +0.14%     
=========================================================
  Files                         47       47              
  Lines                       1612     1666      +54     
  Branches                     365      380      +15     
=========================================================
+ Hits                        1418     1468      +50     
- Misses                       192      196       +4     
  Partials                       2        2

Flag	Coverage Δ
language-server	`98.65% <ø> (ø)`
soql-builder-ui	`92.23% <94.11%> (+0.03%)`	⬆️
soql-model	`80.32% <ø> (ø)`

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

Impacted Files	Coverage Δ
.../modules/querybuilder/customSelect/customSelect.ts	`92.03% <93.65%> (+0.37%)`	⬆️
...l-builder-ui/src/modules/querybuilder/from/from.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 e7e4e68...273211c. Read the comment docs.

…ooling into jhork/custom-select-from

jonnyhork

Some of the behavior described in the PR comments may be hard to envision so if anyone wants videos I can make them.

I think a lot of this work will set us up to use this component for ODER BY and WHERE, hopefully with only changes needed in the parent components from here until we had multi -select support

jonnyhork · 2021-01-27T17:39:37Z

packages/soql-builder-ui/src/index.html

+        --soql-label-container-width: 80px;
+        --soql-selection-container-width: 400px;


Now these names better reflect how the values are used.

jonnyhork · 2021-01-27T20:49:36Z

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

+  width: var(--soql-selection-container-width);
+  min-height: 100px;
+  max-height: 300px;


This will dictate the dimensions of the options menu when visible. I was not sure how tall to make it. I did see some feedback in the issues of the tooling repo that they wish they could see more fields at one time. So maybe we could make it taller if need be?

jonnyhork · 2021-01-27T21:09:49Z

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

+      /*
+      During testing the getter for the displayValue() is called once
+      & before the renderedCallback that caches the inputSelectEl.
+      The only way I have been able to trigger another render of the component
+      is to click the search bar.
+       */


I might want to post a question to the LWC framework channel asking about where the getters fit into the lifecycle of the component. I'm am surprised to see the getter called before the rendered callback and that an element is not in the DOM when we query for it in the renderedCallback()

For now I can do a click event to get everything in place for the test but its not ideal.

I wonder if the gitter is called during the render process. And then when it's finished rendering, we get the renderedCallback()

I could see that. Does that mean that we are not using the right pattern to capture or access elements in the DOM between the getter and rendered callback? Something to consider.

I don't know if there's other options. besides we introduce an isRendered boolean and then check for that everywhere. that could also be messy. i think it's just something we have to deal with.

jonnyhork · 2021-01-27T21:13:47Z

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

+    });
+
+    describe('Dropdown Arrow', () => {
+      it('should toggle the options menu when clicked, with directional arrow', () => {


I thought about breaking this up into 3 tests but found I repeating 90% of the code so I just combined it.

jonnyhork · 2021-01-27T21:20:36Z

packages/soql-builder-ui/src/modules/querybuilder/customSelect/customSelect.ts

+  /*
+  1. If the user is typing, display the searchTerm
+  2. If singleSelect
+    - display the selected value (default)
+    - if there is no searchTerm
+    & the input is in focus
+    & the input is empty
+    display the placeholder
+  */
+  get displayValue(): string {
+    if (this.hasSearchTerm) {
+      return this.searchTerm;
+    }
+
+    if (this.isSingleSelect && this.selectInputEl) {
+      if (!this.selectInputIsFocused || this.selectInputEl.value.length) {
+        return this._value[0] || '';
+      }
+    }
+
+    return '';
+  }


This method is used to control what displayed in the search bar as the value. I need to control the display logic and the state of the component separately.

I found that getting the behavior I wanted was tricky, and this might be something that is just inherently not intuitive. Without || this.selectInputEl.value.length in line 69, when the user would delete the last character of the input then the selected value would immediately repopulate in the input instead of showing the placeholder.

jonnyhork · 2021-01-27T21:24:13Z

packages/soql-builder-ui/src/modules/querybuilder/customSelect/customSelect.ts

+
+      if (this.isSingleSelect) {
+        this._value = [optionValue];
+      } else {
+        this._value = [...this._value, optionValue];
+      }


This might need to change when we want to implement true multi selection but the intention is to maintain state in the component with having to read/write to the model.

jonnyhork · 2021-01-27T21:38:46Z

packages/soql-builder-ui/src/modules/querybuilder/from/from.ts

+  set selected(objectName: string) {
+    this._selectedObject = objectName ? [objectName] : [];


The custom select component accepts an array of selections so I needed to re package the values from the model.

jonnyhork · 2021-01-27T21:39:12Z

packages/soql-data-view/web/queryDataView.css

+  --soql-label-container-width: 80px;
+  --soql-selection-container-width: 400px;


I changed this now so that the dataView and the builder UI are in sync.

jonnyhork · 2021-01-28T16:31:03Z

packages/soql-builder-ui/src/modules/querybuilder/customSelect/customSelect.ts

+  get isSingleSelect() {
+    return !this.multiple;
+  }
+
+  get isMultipleSelect() {
+    return this.multiple;


I realized this api as it is might be a bit odd.
this.multiple is set as boolean value false in the component. However, because LWC won't let us use literals in the template multiple={true} we have to pass in a string multiple="true".
Unfortunately, as it stands, that means that I could put anything as the the string value and it would be coerssed as boolean true so multiple = ''false" passed in from parent to child is also true.

To fix this I would have to set the default value to string "false" and only work with string values "true" and "false"

get isSingleSelect() { return this.multiple === "false" || this.multiple === undefined; }

Or it would depend on the parent to use a getter to pass a boolean value to the child

// in the parent get isMultiple() { return true; } // html multiple={isMultiple}

I do not like the option with a getter in the parent. Sicme we are the only ones using this component at the moment I can see leaving it as is for now. I also don't mind adding logic to check explicitly for strings "true" or "false" but it might be a but verbose for a simple check, idk.
Thoughts @dehru?

Meh, it's currently just us using it. and messing that up would be just us using it wrong.

dehru · 2021-01-28T17:27:20Z

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


+.select__dropdown-arrow {
+  transform: rotate(180deg);
+  margin-right: 2px;


for some reason, I feel the arrow is uncomfortably close to the right. Just a tad. Not a change request, just stating a strange gut reaction.

ya know I had it at 5px originally but I noticed it was further away from the standard element so I changed it to match. I also feel like its a little close but the other thought it that we may want more room once the component is smaller in the WHERE form.

dehru · 2021-01-28T17:45:55Z

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

+      </template>
+      <template if:true={noResultsFound}>
+        <!-- i18n -->
+        <p class="option option--placeholder">No results found.</p>


placeholder is a loaded term. how 'bout option--empty or option--noresults

I went with option--disabled to be more generalizable. That way if we use it for something else we don't have to change the class again.

dehru · 2021-01-28T17:48:20Z

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

+      searchBar.focus();
+      return Promise.resolve().then(() => {
+        const classList = Array.from(searchBar.classList);
+        expect(classList).toContain('select__input-placeholder--fadeout');


It's weird to have css classname changes to cause failing tests. I had that case in my work this week. it's a valid test...not much we can do about it.

dehru · 2021-01-28T17:49:00Z

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

+      return Promise.resolve()
+        .then(() => {
+          optionsWrapperClassList = Array.from(optionsWrapper.classList);
+          expect(optionsWrapperClassList).toContain('options--open');


might think about making this a const since it's used thrice in this test case.

Done. Thanks

dehru · 2021-01-28T17:49:39Z

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

+        })
+        .then(() => {
+          optionsWrapperClassList = Array.from(optionsWrapper.classList);
+          expect(optionsWrapperClassList).not.toContain('options--open');


jonnyhork · 2021-01-28T18:45:53Z

packages/soql-builder-ui/src/modules/querybuilder/customSelect/customSelect.ts

+/** CUSTOM SELECT API
+ *  @attr multiple: string - Toggle single-select or multi-select behavior.
+ *                  NOTE --> Will be interpreted as true if there is any string value passed in.
+ *  @attr is-loading: boolean - Will display 'Loading...' when true.
+ *  @attr all-options: string[] - This the list of all possible options
+ *                                the user can select from.
+ *  @attr selected-options: string[] - If present in single-select, the selectedOption
+ *                                     will be displayed as the value of the input.
+ *                                     The list of options rendered will be all-options - selected-options
+ *  @attr placeholder-text: string - This is the value to be displayed as a placeholder for the input.
+ *  @property value: string[] - Will return the currently selected value(s).
+ *  @event option__selection - This is emitted everytime a valid option is selected.
+ *                             detail { value: optionValue }
+ * **/


I added this to help future dev know how to use this component.

dehru · 2021-01-28T21:04:32Z

This is so good i gotta get it merged asap.

* Jhork/custom select fields (#111) * Jhork/custom select from (#130) * Jhork/custom select order by (#134)

* Fix preview not updating when autosave is off - issue 120 (#125) * add soql-statement to model * verify that we include the soql statement in the model * update the package version so we can publish and consume * Add smarter completion on GROUP BY clause and WHERE literals (#126) * Initial support for Aggregate functions. Also: * Fix: Propose "COUNT()" only if right after "SELECT" * Remove special-case for "SELECT (SELECT ), | FROM Foo". Not worth it * Add completion inside function expression Example: SELECT AVG(|) FROM Also: * Improve ANTLR error recovery to better extract FROM expresssions * Make completions inside function expression type-aware * Add smarter completion on GROUP BY clause Also: * Complete only with "groupable" fields. * Give priority to complete GROUP BY with fields which appear on SELECT and are not aggregated or groupped. * On ORDER BY: Complete only with "sortable" fields * Add completion on semi-join SELECTs (in WHERE clauses) * Fix defect when cursor is around multiple newlines * Improve query analysis impl * Pre-select "WHERE" option if provided * Improve support for queries with semi-joins * Refactor: move soql operator semantics to LSP server-side * Add more code-completion proposals for literal values Including: * Range date literals (LAST_YEAR, YESTERDAY, LAST_N_DAYS:n, etc.) * Simple templates for: int, double, string, currency * Quiet completion on HAVING clause * Add a space before opening parenthesis after INCLUDES/EXCLUDES * New soql lsp version 0.3.3 (#140) * Feature/custom select (#148) * Jhork/custom select fields (#111) * Jhork/custom select from (#130) * Jhork/custom select order by (#134) * set the error overlay to be on top of all other form elements (#150) * set the error overlay to be on top of all other form elements * add z-index to .unsupported-syntax * prepare to publish 0 .0.30 (#151) Co-authored-by: Fernando Dobladez <fernandodobladez@salesforce.com> Co-authored-by: Jonny Hork <jhork@salesforce.com>

* Jhork: Support for Simple WHERE Expressions (#101) * move rule to css common (#108) * Preventing webview re-rendering when soql statement has not changed (#105) * Revert "Disable WHERE in model (#113)" * Type-aware where (#109) * Fix preview not updating when autosave is off - issue 120 (#125) * Recognize recoverable where errors (#116) * Operator and Criteria Error Tooltips (#122) * Support IN, NOT IN, INCLUDES, EXCLUDE operators (#123) * Add smarter completin on GROUP BY clause and WHERE literals (#126) * Feature/custom select (#148) * Jhork/custom select fields (#111) * Jhork/custom select from (#130) * Jhork/custom select order by (#134) * set the error overlay to be on top of all other form elements (#150) * prepare to publish 0 .0.30 (#151) * Feature/where (#154) * Jhork: Support for Simple WHERE Expressions (#101) * move rule to css common (#108) * Preventing webview re-rendering when soql statement has not changed (#105) * Revert "Disable WHERE in model (#113)" * Type-aware where (#109) * Recognize recoverable where errors (#116) * Operator and Criteria Error Tooltips (#122) * Support IN, NOT IN, INCLUDES, EXCLUDE operators (#123) * Revert "Feature/where (#154)" (#157) * Add support for line comments ( `//` ) at the top of SOQL files (#132) * Jhork/where custom select (#161) * Fix: 'undefined' in SOQL string (#162) * Clear tooltip when object changes, fix empty soql statement error blocker (#165) * Track to force render on condition change - fix rebase conflict (#166) * do not send event when user still adding input' (#169) * Jhork/clear modifier field bug fix (#168) * handle a option selection event (#178) * cler tooltip error when X is clicked (#180) * Jhork/workflow updates (#163) * Bump versions for release (#181) Co-authored-by: Jonny Hork <jhork@salesforce.com> Co-authored-by: dehru <599418+dehru@users.noreply.github.com> Co-authored-by: Dehru Cromer <dehru.cromer@salesforce.com> Co-authored-by: Fernando Dobladez <fernandodobladez@salesforce.com>

jonnyhork added 22 commits January 20, 2021 16:20

rotate search glass handle to the right

9574770

use more semantic name for width

e0626dc

width of select input is determined by width of parent

c00118e

search glass icon will have less contrast

25a8c77

adjust layout of custom select component

214ec0a

display selected object as placeholder

c00a64b

fix case, add i18n tag

1456b73

Merge branch 'develop' of github.com:forcedotcom/soql-tooling into jh…

13296f3

…ork/custom-select-from

add multi select mode, only allow one options menu open at a time

0c1a025

add event type

24b9952

use multi select mode

403313c

use custom select component

9522d98

add style for dropdown arrow

51683d6

add dropdown arrow and api for current selected value in single mode

91a7db3

only highlight value if input is clicked

df4a36d

adjust the orientation of the dropdown arrow

ad140d6

add comments

e9dcd62

update test for From component

d80a595

assign the _value to match the model

c856249

add test for placeholder style

7aa0595

finish testing

467fa58

store state of value in multiple mode

fcad4f4

jonnyhork requested review from amphro, code0100fun, dehru, jgellin-sf, mysticflute, praksb and tmock12 as code owners January 27, 2021 17:20

jonnyhork added 3 commits January 27, 2021 10:22

Merge branch 'feature/custom-select' of github.com:forcedotcom/soql-t…

e23c20d

…ooling into jhork/custom-select-from

use variable for container width

a4b5856

close the options menu if there is no value in the input

49fc237

jonnyhork commented Jan 27, 2021

View reviewed changes

handle undefined values

3dfd2b8

jonnyhork commented Jan 28, 2021

View reviewed changes

dehru reviewed Jan 28, 2021

View reviewed changes

jonnyhork added 3 commits January 28, 2021 11:25

add api doc comments

1e827e9

use constant in tests, revert previous change

e7cac52

change class name for option placeholder

84ab692

jonnyhork commented Jan 28, 2021

View reviewed changes

clear the search term in single select mode when menu closes

273211c

dehru merged commit e8f890d into feature/custom-select Jan 28, 2021

dehru deleted the jhork/custom-select-from branch January 28, 2021 21:05

smaddox-sf mentioned this pull request Jan 28, 2021

Suggestion on Fields and WHERE clause boxes #133

Closed

dehru added a commit that referenced this pull request Feb 1, 2021

Feature/custom select (#148)

6810b1b

* Jhork/custom select fields (#111) * Jhork/custom select from (#130) * Jhork/custom select order by (#134)

		--soql-label-container-width: 80px;
		--soql-selection-container-width: 400px;

		set selected(objectName: string) {
		this._selectedObject = objectName ? [objectName] : [];

Jhork/custom select from #130

Jhork/custom select from #130

Uh oh!

Conversation

jonnyhork commented Jan 27, 2021

What does this PR do?

What issues does this PR fix or reference?

Uh oh!

codecov bot commented Jan 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jonnyhork left a comment • 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dehru commented Jan 28, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov bot commented Jan 27, 2021 •

edited

Loading

jonnyhork left a comment •

edited

Loading