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

MGDCTRS-1726 feat: pricing tier additions #662

Merged
merged 1 commit into from Jan 10, 2023

Conversation

gashcrumb
Copy link
Contributor

This change brings the new connector selection list component into the app and applies the pricing tier updates. There's numerous updates and fixes to the connector selection list component, including selection handling, no search results and error state handling. This change also significantly improves the cache updating.

Some known things:

  1. placeholder text in the tier info popup, this awaits text I think
  2. Info pop-up isn't working next to the connector name, there's still some work to be done on the layout of the list item, this will be fixed in a later update as part of that effort
  3. The padding on the right-hand side is to accommodate the "Feedback" thingie

Screenshot from 2022-12-07 14-57-40

@gashcrumb gashcrumb force-pushed the MGDCTRS-1726 branch 4 times, most recently from 9bf8b7e to 1967434 Compare December 8, 2022 21:18
@gashcrumb gashcrumb marked this pull request as draft December 9, 2022 03:31
@gashcrumb
Copy link
Contributor Author

There's a selection issue to look at still, best to hold off merging this until it's sorted.

@gashcrumb gashcrumb marked this pull request as ready for review December 9, 2022 14:54
@gashcrumb
Copy link
Contributor Author

gashcrumb commented Dec 9, 2022

Selection works nicely now, and I added back the selectable item hover behavior, however interestingly enough the help popover will not work out of the box when selection is enabled on the DataList.

@gashcrumb gashcrumb force-pushed the MGDCTRS-1726 branch 4 times, most recently from 0a2f5d6 to 3217ef5 Compare December 9, 2022 19:22
@indraraj
Copy link
Contributor

Greate Work @gashcrumb, the Connector selection page now looks really awesome. 👍

@indraraj
Copy link
Contributor

One scenario where we use both checkbox and filters seems to be creating duplicate cards for the commone connectors. If you want you can merge this PR and work on it as fixes later.

image

@gashcrumb
Copy link
Contributor Author

gashcrumb commented Dec 12, 2022

I'll fix it before merging I think. How did you get this to happen @indraraj? Is that when you've selected something and then interact with the filters afterwards? This is the main area of edge cases still I think 😄

@indraraj
Copy link
Contributor

I'll fix it before merging I think. How did you get this to happen @Indrara

  • Select the source checkbox
    -Then select the featured filter

also, wanted to confirm how combining both the checkbox and the filter behaves. Will they return the union or the intersection of both?

@gashcrumb gashcrumb added the do not merge Something in progress label Dec 12, 2022
@gashcrumb
Copy link
Contributor Author

Adding the do-not-merge label.

It should be the intersection of the filters. So if you check 'source' and 'free' and are in featured, it should show only featured connectors that have both the 'source' and 'free' labels.

@gashcrumb
Copy link
Contributor Author

@mmuzikar FYI this is probably going to have an impact on any automated tests you might have.

@gashcrumb
Copy link
Contributor Author

I added some improvements around selection, I think there's still an edge case and may look at not using the DataList component's selection stuff.

I've found now that the backend is updated that my search query for categories and types don't work as I'd expect, if I check "Source" or "Sink" and select a category, I get all the matches with that category, including the opposite of what I've checked for "Source" or "Sink". For this it turns out the UI is creating a search query like search=(label=category-featured OR label=source} for example, but I think we want either (label=category-featured AND label=source) or with some support on the backend side for the IN operator we could do label in ('category-featured', 'source'). I'll need to sync up with @dhirajsb when he's back from PTO. At the moment, if you select a category and then select a type, then there's no matches, which I think might be reasonable given the SQL being sent.

/*
* label searches could probably use the IN operator
label && label.length > 0
? `label IN (${label
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dhirajsb when you're back, would this be a nicer alternative? I'm not a fan of my current approach :-)

@mmuzikar
Copy link

mmuzikar commented Jan 5, 2023

@gashcrumb I noticed 2 thing whilst playing with this PR:

  1. the numbers of connectors in a category update when the pricing tier is changed (nice), but the same doesn't happen for source/sink - would it be possible to add?
  2. this one is strange :D if you scroll in the list before it's been fully loaded (or at least that's what I assume) then the items you scroll to won't get loaded until you scroll over them 😵 just like this:
    Peek 2023-01-05 09-46.webm

@indraraj indraraj self-requested a review January 5, 2023 10:48
@gashcrumb
Copy link
Contributor Author

gashcrumb commented Jan 5, 2023

the numbers of connectors in a category update when the pricing tier is changed (nice), but the same doesn't happen for source/sink - would it be possible to add?

yep, I think so.

this one is strange :D if you scroll in the list before it's been fully loaded (or at least that's what I assume) then the items you scroll to won't get loaded until you scroll over them dizzy_face just like this

Yes, we're still learning some things about react-virtualized :-) I think the fetching in the cache component needs some more tightening up.

edit - thinking about it more, I guess also this shows an issue that I'm trying to work around in that the API that InfiniteLoader wants to work with is not the API I have available. The cache receives a request for items with a start/end index, and I have an API that supports pages and page sizes. All this mess is around dealing with that, I guess it'd be kinda nice if maybe we could have an option from our APIs to get a specific slice of the data.

@dhirajsb
Copy link

dhirajsb commented Jan 5, 2023

This is a little complicated in SQL, since the where clause has multiple conditions with different values for the same column. It's solvable with other SQL clauses like GROUP BY or INTERSECT. But we need to discuss this in more detail.

@gashcrumb
Copy link
Contributor Author

On the loading I went ahead and created MGDCTRS-1831 as this is something we can fix later I think. And then for the filtering I've gone and created MGDCTRS-1832 to cover updating the UI to use an SQL query that will return the results as we expect. Thankfully @dhirajsb has been looking at this and it sounds like we could have a solution there.

This change brings the new connector selection list component into the
app and applies the pricing tier updates.  There's numerous updates and
fixes to the connector selection list component, including selection
handling, no search results and error state handling.  This change also
significantly improves the cache updating.
@gashcrumb
Copy link
Contributor Author

Ok, rebased and ready for another look with the aforementioned caveats :-)

@gashcrumb gashcrumb marked this pull request as ready for review January 9, 2023 20:54
@gashcrumb gashcrumb removed the do not merge Something in progress label Jan 10, 2023
Copy link
Contributor

@indraraj indraraj left a comment

Choose a reason for hiding this comment

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

tested locally (leaving with mentioned issues), looks fine 👍

@gashcrumb
Copy link
Contributor Author

Awesome, thanks for the 2nd look :-)

@gashcrumb gashcrumb merged commit b2bfa3c into bf2fc6cc711aee1a0c2a:main Jan 10, 2023
@gashcrumb gashcrumb deleted the MGDCTRS-1726 branch January 10, 2023 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants