feat: [sveltekit-pod] Issues Search: inline filters (open/closed) #973

ihardz · 2022-11-28T14:16:25Z

Partially resolves #901

Includes:

inline filters (Open/Closed)
query optimization (current repo is default)

netlify · 2022-11-28T14:16:31Z

✅ Deploy Preview for jazzy-gecko-7ceb2c canceled.

Name	Link
🔨 Latest commit	`1c1d0d8`
🔍 Latest deploy log	https://app.netlify.com/sites/jazzy-gecko-7ceb2c/deploys/638693837531ac000a196ce3

netlify · 2022-11-28T14:16:31Z

✅ Deploy Preview for elaborate-baklava-440c0a canceled.

Name	Link
🔨 Latest commit	`1c1d0d8`
🔍 Latest deploy log	https://app.netlify.com/sites/elaborate-baklava-440c0a/deploys/638693837243d20008af44f7

netlify · 2022-11-28T14:16:41Z

✅ Deploy Preview for lucent-kleicha-c0913e canceled.

Name	Link
🔨 Latest commit	`1c1d0d8`
🔍 Latest deploy log	https://app.netlify.com/sites/lucent-kleicha-c0913e/deploys/638693837243d20008af44f5

netlify · 2022-11-28T14:16:44Z

✅ Deploy Preview for superb-nasturtium-3dbf18 canceled.

Name	Link
🔨 Latest commit	`1c1d0d8`
🔍 Latest deploy log	https://app.netlify.com/sites/superb-nasturtium-3dbf18/deploys/638693832a516c000905ab74

netlify · 2022-11-28T14:16:46Z

✅ Deploy Preview for remarkable-caramel-0ba9f6 canceled.

Name	Link
🔨 Latest commit	`1c1d0d8`
🔍 Latest deploy log	https://app.netlify.com/sites/remarkable-caramel-0ba9f6/deploys/6386938351ff700009dc4121

aws-amplify-us-east-1 · 2022-11-28T14:19:45Z

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-973.dbm2zhgk5abrj.amplifyapp.com

aws-amplify-us-east-1 · 2022-11-28T14:20:44Z

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-973.d3sqkonra6x7y9.amplifyapp.com

TapaiBalazs

I'm not acquainted with svelte, but I have some coments/questions on this PR. Please answer them. :)

svelte-kit-scss/src/lib/components/IssueSearch/IssueSearchControls/IssueSearchControls.svelte

TapaiBalazs · 2022-11-28T14:36:02Z

svelte-kit-scss/src/lib/components/IssueSearch/IssueSearchControls/IssueSearchControls.svelte

+      on:select={async ({ detail }) => await handleFilterSelect(detail)}
+    >
+      <svelte:fragment slot="option-icon-left" let:option>
+        {#each [resolveStateFilterIssueState(option)] as state}


Do I understand this correctly?

You put one item into an array to iterate over it.

This array will always have one item which can be undefined or a truthy value

If it is truthy it renders an icon.

This seems to be something that's a bit overengineered, isn't it possible to only use one if statement here instead of iterating over an array that is created every time? 🤔

svelte doesnt support in-template scoped variables at the moment.
sveltejs/svelte#1521

Huh, that's inconvenient.

Yeah, the framework grows fast still... And they will to solve the problem i think

But I wouldnt like to upgrade the version at the moment. Thoughts?

Well leave it this way then, but this is something that I think is very ugly. :\

I think the same..

svelte-kit-scss/src/lib/components/shared/buttons/SelectButton.svelte

TapaiBalazs · 2022-11-28T14:45:09Z

svelte-kit-scss/src/lib/components/shared/buttons/SelectButton.svelte

+  type TOption = $$Generic;
+  export let options: TOption[];
+  export let labelAccessor = (option: TOption) => String(option);
+  export let checkedPredicate: (option: TOption) => boolean = () => false;


correct me if I'm wrong, but this always returns false and does not really take an option. Is that intended?

It makes the checkedPredicate input property as optional (with default value)
It is a common practice for reusable components

So, these are like component properties?

In angular these would be @Input() decorators?

Yes. Absolutely the same

TapaiBalazs · 2022-11-28T15:01:01Z

svelte-kit-scss/src/lib/helpers/issues-search-query-builder.ts

@@ -33,6 +33,17 @@ export const splitFilterParameters = (query: string): string[] => {
  }, [] as string[]);
 };

+export const ensureRepoParameter = (query: string, repoName: string): string => {
+  const repoParam = splitFilterParameters(query).find((x) =>


I have a feeling that this part is rather overengineered here. I don't really understand what needs to be done here. Are there any tests for these? 🤔

It checks if the repo parameter in the query. If not we have to add current one. It is how it works in native github. To check that just try to play with issues search in native github...

I kind of wanted to see where you ended up with these, but it seems to me like we really need a few things:

A type of object that represents the search filters, I think this would include the PREDEFINED_SEARCH_QUERY_PARAMETER_GROUPS you have defined, and also the parametrized options like repo:, is:issue, milestone:, sort: and whatever else.

Then we would have a function that can convert from a string to an object of that type, and from an object of that type to a string.

If you had those 2 things separate, and tested, then I think you could clean up a lot of this code, because it would most involve creating URLs from the state objects, and parsing the current route into one of those objects.

What do you think?

The object model and builder will be too complex this way

But I would do this with pleasure if we will have time

It seems overly complex now, and spread out.

I was hoping it would make it simpler and "all in one place"

Should I make it more reusable?

BigAB · 2022-11-28T17:40:50Z

svelte-kit-scss/src/lib/components/IssueSearch/IssueSearchControls/IssueSearchControls.svelte

-    <DropdownFilterTextButton text="Sort" />
-  </DropdownMenuSelect>
+  <div class="primary">
+    <SelectButton


This seems to be unnecessarily complex for what it is.

We've effectively got 2 "tabs", or links, open and closed, and one of them is currently active.

What you have here seems very very configurable and extendible maybe, but unnecessarily so.

I think you maybe need to take a step back here, and just have the 2 links, Open and Closed, be always visible here with their Icons hardcoded, and use the state filters to provide the 2 links with the label, the href and whether or not they have an active class applied.

Motivation: The purpose of the showcase is to show how to solve the differnet kind of problems with svelte and sveltekit.
On typical real life projects there are Frontend and Backend team.
This aproach gives an ability to separate backend and frontend part of work.

I created a separate components for all the things. Every components has as less responsibilities as possible in order to follow SOLID and DRY principles

I think the best sveltekit application is like a constructor:

Yeah, but as a showcase, for someone to look at and understand, "oh this is what a SvelteKit app would look like", this doesn't fulfil the need.

This is hard for me to understand, and might give someone the impression that SvelteKit "requires" or demands code like this for very simple things, which it does not.

I would prefer anyone unfamiliar with Svelte or SvelteKit be able to look at this code and immediately recognize what part of the app it is from and how it works.

Could you please help me to understand what is my bad? It looks very simple to me. I tried to use as much svelte features instead of hardcoding.
Lets clarify... What the snippet is most hard to understand?

Should I make it less reusable?

Okay. Now I got your point,
But have some confuses:

this approach will rerun the backend code each time you hover on the link (To reproduce, just add console.log to page load function)

It will require styles duplication.

if the filters will be returned in different order - it will cause wrong look

Are you ok with that? Should I remove SelectButton component?

this approach will rerun the backend code each time you hover on the link (To reproduce, just add console.log to page load function)

this will cause us to often get github throttling error

this approach will rerun the backend code each time you hover on the link (To reproduce, just add console.log to page load function)

This is a "feature" of SvelteKits prefetching we've opted into on the body, which we can (and maybe should) turn off by removing this data-sveltekit-prefetch atrttibute:

<body data-sveltekit-prefetch>

You should also be able to opt out for certain links and sections by adding a data-sveltekit-preload-data="off" attribute to the link or a containing element but that doesn't seem to work (though it should, so a sveltekit bug?)

BigAB · 2022-11-28T17:59:12Z

svelte-kit-scss/src/lib/helpers/issues-search-query-builder.ts

@@ -33,6 +33,17 @@ export const splitFilterParameters = (query: string): string[] => {
  }, [] as string[]);
 };

+export const ensureRepoParameter = (query: string, repoName: string): string => {
+  const repoParam = splitFilterParameters(query).find((x) =>


I kind of wanted to see where you ended up with these, but it seems to me like we really need a few things:

A type of object that represents the search filters, I think this would include the PREDEFINED_SEARCH_QUERY_PARAMETER_GROUPS you have defined, and also the parametrized options like repo:, is:issue, milestone:, sort: and whatever else.

Then we would have a function that can convert from a string to an object of that type, and from an object of that type to a string.

If you had those 2 things separate, and tested, then I think you could clean up a lot of this code, because it would most involve creating URLs from the state objects, and parsing the current route into one of those objects.

What do you think?

BigAB

I think this is mergeable, we can look to further refactoring in the future

* Dinamic filters: Milestone filter. Dropdown adjustments * fix label * Milestone filter adjustments * Code review fixes

* Dinamic filters: Milestone filter. Dropdown adjustments * fix label * Milestone filter adjustments * resolve redirect for issues search pages * adjust linked components * adjust linked components * Code review fixes * Code review * Code review

…hub.com/thisdot/starter.dev-github-showcases into 901-sveltekit-pod-issues-inline-filters

BigAB

[sveltekit-pod] Issues Search: inline filters (open/closed)

f0f4312

ihardz requested review from BigAB, flakolefluk, TapaiBalazs and iansamz November 28, 2022 14:16

ihardz self-assigned this Nov 28, 2022

TapaiBalazs reviewed Nov 28, 2022

View reviewed changes

BigAB suggested changes Nov 28, 2022

View reviewed changes

Code review fixes

c4a2d3b

ihardz requested review from BigAB and TapaiBalazs November 28, 2022 18:42

Code review + Components cleanup

9dbed2c

BigAB previously approved these changes Nov 29, 2022

View reviewed changes

feat: [sveltekit pod] missing issues filters (#979)

5c07370

* Dinamic filters: Milestone filter. Dropdown adjustments * fix label * Milestone filter adjustments * Code review fixes

ihardz dismissed BigAB’s stale review via 5c07370 November 29, 2022 23:05

BigAB previously approved these changes Nov 29, 2022

View reviewed changes

ihardz added 2 commits November 30, 2022 02:18

Code review fixes

45367cb

Merge branch '901-sveltekit-pod-issues-inline-filters' of https://git…

1c1d0d8

…hub.com/thisdot/starter.dev-github-showcases into 901-sveltekit-pod-issues-inline-filters

ihardz dismissed BigAB’s stale review via 1c1d0d8 November 29, 2022 23:19

BigAB approved these changes Nov 29, 2022

View reviewed changes

ihardz merged commit bc543e7 into main Nov 29, 2022

ihardz deleted the 901-sveltekit-pod-issues-inline-filters branch November 29, 2022 23:23

feat: [sveltekit-pod] Issues Search: inline filters (open/closed) #973

feat: [sveltekit-pod] Issues Search: inline filters (open/closed) #973

Conversation

ihardz commented Nov 28, 2022

netlify bot commented Nov 28, 2022 • edited Loading

✅ Deploy Preview for jazzy-gecko-7ceb2c canceled.

netlify bot commented Nov 28, 2022 • edited Loading

✅ Deploy Preview for elaborate-baklava-440c0a canceled.

netlify bot commented Nov 28, 2022 • edited Loading

✅ Deploy Preview for lucent-kleicha-c0913e canceled.

netlify bot commented Nov 28, 2022 • edited Loading

✅ Deploy Preview for superb-nasturtium-3dbf18 canceled.

netlify bot commented Nov 28, 2022 • edited Loading

✅ Deploy Preview for remarkable-caramel-0ba9f6 canceled.

aws-amplify-us-east-1 bot commented Nov 28, 2022

aws-amplify-us-east-1 bot commented Nov 28, 2022

TapaiBalazs 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

Choose a reason for hiding this comment

ihardz Nov 29, 2022 • 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

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

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

ihardz Nov 28, 2022 • 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

ihardz Nov 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ihardz Nov 28, 2022 • edited Loading

Choose a reason for hiding this comment

BigAB Nov 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BigAB left a comment

Choose a reason for hiding this comment

BigAB left a comment

Choose a reason for hiding this comment

netlify bot commented Nov 28, 2022 •

edited

Loading

netlify bot commented Nov 28, 2022 •

edited

Loading

netlify bot commented Nov 28, 2022 •

edited

Loading

netlify bot commented Nov 28, 2022 •

edited

Loading

netlify bot commented Nov 28, 2022 •

edited

Loading

ihardz Nov 29, 2022 •

edited

Loading

ihardz Nov 28, 2022 •

edited

Loading

ihardz Nov 28, 2022 •

edited

Loading

ihardz Nov 28, 2022 •

edited

Loading

BigAB Nov 28, 2022 •

edited

Loading