feat(ui): check if project exists when searching #1030

xla · 2020-10-13T21:29:57Z

Up until now we assumed the provided urn is not present in the monorepo.
This change alters the behaviour to first check if a project exists
locally - because locally sourced is best source. If it exists locally
the result box offers to navigate directly to it -- also possible with a
hit of enter. Should it not exist a track toggle is presented, which
when pressed will fire a request for the project and show a
notification.

check for project locally first
navigate to existing project
request urn from the network on track toggle
validate input to be a valid urn

Refs #984

cloudhead · 2020-10-13T22:03:29Z

Concept ACK

NunoAlexandre · 2020-10-14T07:03:38Z

ui/App.svelte

-  import Modal from "./Modal";
+  import ModalNewProject from "./Modal/NewProject.svelte";
+  import ModalSearch from "./Modal/Search.svelte";
+  import ModalShortcuts from "./Modal/Shortcuts.svelte";


It would be better to separate such cosmetic changes into separates PRs. And I'd prefer to undo this.

This is not a cosmetic change, it's a structural one. Given the overhead of getting a change reviewed, approved and kept in sync with mainline, I made the call to include it here. As I touched the part of the codebase already. It does not distract or obfuscate the other changes and it should be fine to approve it along the rest.

Sorry for the wrong categorization on my side. In any case, still not fond of this change. What's the context and motivation here?

It's the removal of the unnecessary bookkeeping in index.js plus dropping the superfluous Modal suffix.

Yeah, I don't dislike dropping the bookkepping but the namespacing I do appreciate. The prefix is staying around as you see, though less elegant. Happy to talk about this another time.

NunoAlexandre · 2020-10-14T07:04:11Z

ui/DesignSystem/Component/TrackToggle.svelte

@@ -1,6 +1,8 @@
-<script lang="ts">
+<script lang="typescript">


Suggested change

<script lang="typescript">

<script lang="ts">

Unless there's a good reason for the change, let's keep it consistent.

#1030 (comment)

Ah cool 👍

NunoAlexandre · 2020-10-14T07:05:19Z

ui/DesignSystem/Component/TrackToggle.svelte

-  enum TrackingEvent {
-    Track = "track",
-    Untrack = "untrack",
-  }
-


Kind of unnecessary to move this to a new module with only this type when it is only used here...

Consumers rely on the string value of these variants to catch the right events, but I didn't find a way to share that meaningfully. This was an attempt to do so.

fwiw: I'm all for small modules with specific data 👍 Just because it's only used here now doesn't mean the tests of time will not force it to be used elsewhere or be added to.

NunoAlexandre · 2020-10-14T07:05:34Z

ui/DesignSystem/Primitive/Input/Text.svelte

-<script lang="ts">
-  import { createEventDispatcher } from "svelte";
-
+<script lang="typescript">


Suggested change

<script lang="typescript">

<script lang="ts">

NunoAlexandre · 2020-10-14T07:06:15Z

ui/Hotkeys.svelte

@@ -1,4 +1,4 @@
-<script lang="ts">
+<script lang="typescript">


Suggested change

<script lang="typescript">

<script lang="ts">

NunoAlexandre · 2020-10-14T07:06:45Z

ui/Modal/Search.svelte

@@ -0,0 +1,171 @@
+<script lang="typescript">


Suggested change

<script lang="typescript">

<script lang="ts">

NunoAlexandre · 2020-10-14T07:07:41Z

ui/Modal/Search.svelte

+  let id: string;
+  let value: string;


could you document what these are?

Could you expand on what kind of documentation you expect when stumbling upon these? Also would it help to rename them to not require documentation?

Yeah, renaming would probably be best. Expanding, I open this file and have no idea what id and value stand or are used for.

Looking at the code I'd probably call this rawUrn or unvalidatedUrn.

Also is the id above actually the urn? I find the name id hard to discern whether it's a PeerId or a RadUrn. WHERE ARE MY TYPES AS DOCUMENTATION /raises fist to sky

Looking at the code I'd probably call this rawUrn or unvalidatedUrn.
It's validated.

It's the id part of of a RadUrn.

ui/src/search.ts

rudolfs · 2020-10-14T07:16:48Z

ui/DesignSystem/Component/TrackToggle.svelte

@@ -1,6 +1,8 @@
-<script lang="ts">
+<script lang="typescript">


Just out of curiosity, what's the difference? So far we've been using ts everywhere.
Official docs also mention ts: https://svelte.dev/blog/svelte-and-typescript#1_Compiling_TypeScript.

Somehow my editor stack doesn't pick it up unless it's the full language name.

Whirrd!

I think we should try to keep them consistent, so we should either fix your editor or update the rest of the codebase to use typescript.

NunoAlexandre · 2020-10-14T07:33:23Z

ui/Modal/Search.svelte

+  .container {
+    width: 26.5rem;
+  }
+
+  .search-bar {
+    margin-bottom: 1rem;
+    position: relative;
+  }


Suggested change

.container {

width: 26.5rem;

}

.search-bar {

margin-bottom: 1rem;

position: relative;

}

.container {

width: 50vw;

max-width: 800px;

min-width: 500px;

}

.search-bar {

margin-bottom: 1rem;

position: relative;

padding: var(--content-padding);

background-color: var(--color-background);

border-radius: 4px;

}

Now the validation error message is displaying in red on top of the overlay which is dark, looking no good, imo:

This suggestion does two things:

Improves the responsiveness of the modal container, looking better in both small and large screens:

small

medium

large

Wraps the search bar with the appropriate background colour with nice padding:

Thanks for the suggestion, @radicle-dev/product is aware of it and the work is tracked in #1031

Woudln't hurt to include this alrerady though as an temporary improvement.

We can make these changes when polishing for the beta release. Don't see much value in doing changes until @radicle-dev/product has settled on something.

Co-authored-by: Nuno Alexandre <hi@nunoalexandre.com>

FintanH · 2020-10-14T08:35:24Z

ui/Modal/Search.svelte

+  import { Icon, Input } from "../DesignSystem/Primitive";
+  import { Remote, TrackToggle } from "../DesignSystem/Component";
+
+  let id: string;


Wait, what's the id used for? I see that upon validation you set it as the Urn minus rad:git, but I can't see where that's used 🤔

It's shown next to the track button in the search resullt.

FintanH · 2020-10-14T08:40:57Z

ui/src/search.ts

+
+    case Kind.RequestProject:
+      projectRequestStore.loading();
+      // FIXME(xla): This truly belongs in project.ts.


Or project/search.ts

xla added 2 commits October 13, 2020 18:23

Make modals consistent

ed9fc1d

Check if project exists

1e4331a

xla added feature Something that doesn't exist yet ui labels Oct 13, 2020

xla requested review from cloudhead, rudolfs, juliendonck, FintanH, brandonhaslegs and NunoAlexandre October 13, 2020 21:29

xla self-assigned this Oct 13, 2020

xla added 2 commits October 13, 2020 23:33

Cast my data

b96a5c5

Merge branch 'master' into xla/984-check-project

2a0359b

xla mentioned this pull request Oct 13, 2020

Expand Search design #1031

Closed

Be still my clippy

27d3c02

xla added 2 commits October 14, 2020 08:04

Fix keyboard navigation in onboarding

a5dfb32

Types types types

5f30e35

xla mentioned this pull request Oct 14, 2020

Wire up waiting room with search #984

Closed

8 tasks

NunoAlexandre reviewed Oct 14, 2020

View reviewed changes

rudolfs reviewed Oct 14, 2020

View reviewed changes

NunoAlexandre reviewed Oct 14, 2020

View reviewed changes

xla and others added 2 commits October 14, 2020 10:03

Update ui/src/search.ts

bdc78b2

Co-authored-by: Nuno Alexandre <hi@nunoalexandre.com>

Update ui/src/search.ts

ec349ce

Co-authored-by: Nuno Alexandre <hi@nunoalexandre.com>

xla mentioned this pull request Oct 14, 2020

Settle on lang tag for components #1032

Closed

FintanH reviewed Oct 14, 2020

View reviewed changes

Merge branch 'master' into xla/984-check-project

7166f28

NunoAlexandre approved these changes Oct 14, 2020

View reviewed changes

rudolfs approved these changes Oct 14, 2020

View reviewed changes

xla merged commit 646c50e into master Oct 14, 2020

xla deleted the xla/984-check-project branch October 14, 2020 11:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ui): check if project exists when searching #1030

feat(ui): check if project exists when searching #1030

xla commented Oct 13, 2020

cloudhead commented Oct 13, 2020

NunoAlexandre Oct 14, 2020 •

edited

xla Oct 14, 2020

NunoAlexandre Oct 14, 2020

xla Oct 14, 2020

NunoAlexandre Oct 14, 2020

NunoAlexandre Oct 14, 2020

xla Oct 14, 2020

NunoAlexandre Oct 14, 2020

NunoAlexandre Oct 14, 2020

xla Oct 14, 2020

FintanH Oct 14, 2020

NunoAlexandre Oct 14, 2020

NunoAlexandre Oct 14, 2020

NunoAlexandre Oct 14, 2020

NunoAlexandre Oct 14, 2020

xla Oct 14, 2020

NunoAlexandre Oct 14, 2020

FintanH Oct 14, 2020

xla Oct 14, 2020

rudolfs Oct 14, 2020

xla Oct 14, 2020

rudolfs Oct 14, 2020

xla Oct 14, 2020

NunoAlexandre Oct 14, 2020 •

edited

xla Oct 14, 2020

NunoAlexandre Oct 14, 2020

xla Oct 14, 2020

FintanH Oct 14, 2020

xla Oct 14, 2020

FintanH Oct 14, 2020

feat(ui): check if project exists when searching #1030

feat(ui): check if project exists when searching #1030

Conversation

xla commented Oct 13, 2020

cloudhead commented Oct 13, 2020

NunoAlexandre Oct 14, 2020 • edited

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

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

NunoAlexandre Oct 14, 2020 • edited

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

NunoAlexandre Oct 14, 2020 •

edited

NunoAlexandre Oct 14, 2020 •

edited