fix: onchange for autocomplete in datafilters #2047

benlister-okta · 2023-11-29T01:10:27Z

Fix to add onChange event for Autocomplete in DataFilters for DataTable.

Jira: OKTA-671006

Summary

Logic added for OnChange to trigger updates in the Autocomplete filter in DataFilters. This was previously placeholder/stub logic.
The filter now works like the Textfield and Number filters where the user makes a selection and click a primary button to add the filter.

Testing & Screenshots

N/A

Fix to add `onChange` event for Autocomplete in DataFilters for DataTable

bryancunningham-okta · 2023-12-01T18:48:20Z

packages/odyssey-react-mui/src/labs/DataFilters.tsx

+                                ] as string) ?? ""
+                              }
+                              onChange={(_, value) => {
+                                const label =


@benlister-okta Can we pull this logic up into the handleInputChange?

Never mind, don't think we can :( How about making another function like... getAutoCompleteLabel or something like that? Generally, I just try to avoid having to much logic inside JSX

It would be cleaner if we did for sure. I was exploring this today and the issue I run into is that due to the structure of Autocomplete's data which simulates a Select, the check to see if the data is an object or a string is needed here, but not in the global handleInputChange. I might be missing something, let's chat about this Monday!

@bryancunningham-okta I pulled back my last change and went with your idea of moving the logic to a getAutoCompleteLabel function. Looking at the changes I had in handleInputChange, it felt like it was getting too complicated overall. Let me know what you think of this approach or if you think and could be further optimized.

Type checks in onChange for Autocomplete moved into handleInputChange

packages/odyssey-react-mui/src/labs/DataFilters.tsx

Created a new function called getAutoCompleteLabel to simplify the Autocomplete logic

benlister-okta · 2023-12-07T19:24:12Z

@bryancunningham-okta @KevinGhadyani-Okta @jordankoschei-okta let me know if you have thoughts on this approach. I think abstracting the checks into the autocomplete-specific function will be cleaner than what I was trying to do previously.

bryancunningham-okta · 2023-12-07T21:01:21Z

packages/odyssey-react-mui/src/labs/DataFilters.tsx

+  ): string => {
+    if (Array.isArray(value)) {
+      // Iterating to find the label
+      for (const valueElement of value) {


Is the value array ever more than one entry?

I didn't explore this case but multi-select is an option for Autocomplete

So, it could theoretically be something like this?

[{label: 'some label'}, 'another label']

bryancunningham-okta · 2023-12-07T21:07:28Z

packages/odyssey-react-mui/src/labs/DataFilters.tsx

+          return valueElement.label;
+        }
+      }
+    } else if (


We could probably just do

} else if (value?.label) {...

removes nested ternary and breaks out handleInputChange into 2 steps

benlister-okta · 2024-01-19T00:14:58Z

Hey @bryancunningham-okta @jordankoschei-okta, @KevinGhadyani-Okta helped me refactor some of the logic for the Autocomplete filter's onChange and I think this is ready for another review.

Also, I just realized I still haven't gained access to Aplitools so I need help with the failed visual regression test (I believe it timed out).

benlister-okta · 2024-01-19T00:18:13Z

packages/odyssey-react-mui/src/labs/DataFilters.tsx

+                                      value: undefined,
+                                    });
+
+                                    updateFilters({


Kevin and I also discussed splitting the logic for updating the filter value shown in the DataFilter UI and the actual value in the form components into 2 parts updateInputValue and updateFilters to separate concerns and make the code a little easier to read.

unnecessary dependency was included

Fix to add `onChange` event for Autocomplete in DataFilters for DataTable

Type checks in onChange for Autocomplete moved into handleInputChange

Created a new function called getAutoCompleteLabel to simplify the Autocomplete logic

removes nested ternary and breaks out handleInputChange into 2 steps

unnecessary dependency was included

…TA-671006

packages/odyssey-react-mui/src/Radio.tsx

KevinGhadyani-Okta · 2024-01-29T16:46:44Z

packages/odyssey-react-mui/src/Radio.tsx

+  useImperativeHandle(
+    inputFocusRef,
+    () => {
+      const element = ref.current;


Why create a temp variable? I think inputRef.current?.focus() is fine. The .current will always exist, it just might be run depending on when it's run.

@KevinGhadyani-Okta the scope of my PR was only to add logic for autocomplete in DataFilters. I am not sure what the reason for using a temp variable here was originally, but because I am unfamiliar with this code, I am unsure how to test it. Do you think we should consider this change in a future PR since it adds scope?

KevinGhadyani-Okta · 2024-01-29T16:49:08Z

packages/odyssey-react-mui/src/Radio.tsx


 const Radio = ({
+  inputFocusRef,


Is the plan that any bi-directional props (like blur() and focus() are gonna have separate ref props?

If not, we should instead make this more generic so consumers only have to worry about creating a single ref to pass.

I would rename this more generically such as inputRef.

I am also not certain on this one, I believe the updates to Radio in the PR are from a library update, not related to new functionality.

KevinGhadyani-Okta

Did a rough check. Are all these changes yours? Did you need to merge main?

KevinGhadyani-Okta · 2024-01-30T20:25:15Z

packages/odyssey-react-mui/src/labs/DataFilters.tsx

+          if (typeof valueElement !== "string") {
+            return valueElement.label;
+          }
+
+          return;


I'm surprised this doesn't throw a typescript error because we're not returning the same thing. Either we're returning a string or undefined.

I wonder if it would be better to say "if it's a string, then return undefined; otherwise, return valueElement.label". I say that because I prefer the positive over the negative with if statements:

Suggested change

if (typeof valueElement !== "string") {

return valueElement.label;

}

return;

if (typeof valueElement === "string") {

return undefined;

}

return valueElement.label;

KevinGhadyani-Okta · 2024-01-30T20:25:55Z

packages/odyssey-react-mui/src/labs/DataFilters.tsx

+                        <Box
+                          sx={{
+                            display: "flex",
+                            gap: 2,
+                            alignItems: "flex-end",
+                          }}
+                        >
+                          <Box sx={{ width: "100%" }}>


Were we going to add styled components in this PR or a followup?

thanks for the reminder, PR is updated with this now.

KevinGhadyani-Okta · 2024-01-30T20:26:57Z

packages/odyssey-react-mui/src/labs/DataFilters.tsx

+                              value={
+                                inputValues[filterPopoverCurrentFilter.id] ?? ""
+                              }
+                              onChange={(_, value) => {


It's fine to keep this labeled as event even if we don't use it because if we do use it, the _ isn't very helpful:

Suggested change

onChange={(_, value) => {

onChange={(event, value) => {

If it's an ESLint error, don't worry about it.

changing to event gives an ESLint error

KevinGhadyani-Okta · 2024-01-30T20:28:55Z

packages/odyssey-react-mui/src/labs/DataFilters.tsx

+                              options={filterPopoverCurrentFilter.options.map(
+                                (option: { label: string }) => ({
+                                  label: option.label,
+                                })
+                              )}


We need to memoize this value.

Right now, we're creating options every time a re-render occurs which is anytime you click something in DataFilters, but I don't think the options ever change once this is rendered unless we're removing selected filters?

I thought there was something @jordankoschei-okta did recently about making it so selected items stay in the Autocomplete list.

@KevinGhadyani-Okta Are you open to pairing on this? It looks like we use memo elsewhere in this file but I am not familiar with how the library works.

fix: onchange for autocomplete in datafilters

5e831cd

Fix to add `onChange` event for Autocomplete in DataFilters for DataTable

benlister-okta requested review from KevinGhadyani-Okta, benschell-okta and chrispulsinelli-okta as code owners November 29, 2023 01:10

benlister-okta requested a review from a team November 29, 2023 01:10

bryancunningham-okta reviewed Dec 1, 2023

View reviewed changes

fix: decouple onChange logic

75d553a

Type checks in onChange for Autocomplete moved into handleInputChange

bryancunningham-okta reviewed Dec 4, 2023

View reviewed changes

packages/odyssey-react-mui/src/labs/DataFilters.tsx Outdated Show resolved Hide resolved

refactor: move autocomplete logic

d2a9d23

Created a new function called getAutoCompleteLabel to simplify the Autocomplete logic

bryancunningham-okta reviewed Dec 7, 2023

View reviewed changes

benlister-okta added 2 commits December 19, 2023 13:19

refactor: reduce getAutoCompleteLabel complexity

d670a81

refactor: clean up logic for auto complete filter

70a38a1

removes nested ternary and breaks out handleInputChange into 2 steps

benlister-okta commented Jan 19, 2024

View reviewed changes

benlister-okta added 8 commits January 22, 2024 10:33

fix: fix warning in DataFilters file

d82cdde

unnecessary dependency was included

fix: onchange for autocomplete in datafilters

e82bbd5

Fix to add `onChange` event for Autocomplete in DataFilters for DataTable

fix: decouple onChange logic

d26bbc2

Type checks in onChange for Autocomplete moved into handleInputChange

refactor: move autocomplete logic

b23879a

Created a new function called getAutoCompleteLabel to simplify the Autocomplete logic

refactor: reduce getAutoCompleteLabel complexity

324bd5f

refactor: clean up logic for auto complete filter

05b8879

removes nested ternary and breaks out handleInputChange into 2 steps

fix: fix warning in DataFilters file

5fdecf1

unnecessary dependency was included

Merge branch 'OKTA-671006' of https://github.com/okta/odyssey into OK…

f52021e

…TA-671006

KevinGhadyani-Okta reviewed Jan 29, 2024

View reviewed changes

packages/odyssey-react-mui/src/Radio.tsx Outdated Show resolved Hide resolved

KevinGhadyani-Okta reviewed Jan 29, 2024

View reviewed changes

packages/odyssey-react-mui/src/Radio.tsx Outdated Show resolved Hide resolved

KevinGhadyani-Okta reviewed Jan 29, 2024

View reviewed changes

benlister-okta changed the base branch from main to ti-textfield-support-inputmode January 29, 2024 19:04

benlister-okta changed the base branch from ti-textfield-support-inputmode to main January 29, 2024 19:04

benlister-okta requested a review from KevinGhadyani-Okta January 29, 2024 19:04

KevinGhadyani-Okta reviewed Jan 30, 2024

View reviewed changes

fix: implement requested PR changes

6383c3b

benlister-okta requested a review from KevinGhadyani-Okta January 30, 2024 23:05

fix: add memo to autocomplete options

c6c0d5d

KevinGhadyani-Okta approved these changes Feb 1, 2024

View reviewed changes

benlister-okta merged commit 34e9086 into main Feb 1, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: onchange for autocomplete in datafilters #2047

fix: onchange for autocomplete in datafilters #2047

benlister-okta commented Nov 29, 2023

bryancunningham-okta Dec 1, 2023

bryancunningham-okta Dec 1, 2023

benlister-okta Dec 2, 2023

benlister-okta Dec 4, 2023

benlister-okta commented Dec 7, 2023

bryancunningham-okta Dec 7, 2023

benlister-okta Dec 7, 2023

bryancunningham-okta Dec 7, 2023

bryancunningham-okta Dec 7, 2023

benlister-okta commented Jan 19, 2024 •

edited

Loading

benlister-okta Jan 19, 2024

KevinGhadyani-Okta Jan 29, 2024

benlister-okta Jan 29, 2024

KevinGhadyani-Okta Jan 29, 2024

benlister-okta Jan 29, 2024

KevinGhadyani-Okta left a comment

KevinGhadyani-Okta Jan 30, 2024

KevinGhadyani-Okta Jan 30, 2024

benlister-okta Jan 30, 2024

KevinGhadyani-Okta Jan 30, 2024 •

edited

Loading

benlister-okta Jan 30, 2024

KevinGhadyani-Okta Jan 30, 2024

benlister-okta Jan 30, 2024

fix: onchange for autocomplete in datafilters #2047

fix: onchange for autocomplete in datafilters #2047

Conversation

benlister-okta commented Nov 29, 2023

Summary

Testing & Screenshots

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benlister-okta commented Dec 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benlister-okta commented Jan 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KevinGhadyani-Okta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KevinGhadyani-Okta Jan 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benlister-okta commented Jan 19, 2024 •

edited

Loading

KevinGhadyani-Okta Jan 30, 2024 •

edited

Loading