Skip to content
This repository has been archived by the owner on Aug 1, 2022. It is now read-only.

feat(ui): onboarding refresh #837

Merged
merged 40 commits into from
Sep 4, 2020
Merged

feat(ui): onboarding refresh #837

merged 40 commits into from
Sep 4, 2020

Conversation

rudolfs
Copy link
Member

@rudolfs rudolfs commented Aug 28, 2020

Note: I couldn't get the outtro animation working, it's breaking routing and I don't know how to fix it at the moment.

Closes: #657 #623.

onboarding

@rudolfs rudolfs self-assigned this Aug 28, 2020
@rudolfs rudolfs changed the title style(ui): onboarding style refresh feat(ui): onboarding refresh Aug 31, 2020
fullMessages: false,
};

validatejs.validators.optional = (value, options) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we move validation logic out of the component, we can unit test it & also maintain consistency in how we validate

Copy link
Member Author

Choose a reason for hiding this comment

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

Converted the NameEntry component.
Couldn't do it for PasswordEntry because the validation fields there depend on each other and our current validations.ts wrapper doesn't support that.

@rudolfs rudolfs marked this pull request as ready for review September 3, 2020 09:38
Copy link
Contributor

@MeBrei MeBrei left a comment

Choose a reason for hiding this comment

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

I added some small comments in line. Will click through the flow now.

If you have time in this PR, you could change the px to rem #303

cypress/integration/onboarding.spec.js Outdated Show resolved Hide resolved
ui/Screen/Onboarding/Welcome.svelte Outdated Show resolved Hide resolved
ui/src/identity.ts Show resolved Hide resolved
Copy link
Contributor

@MeBrei MeBrei left a comment

Choose a reason for hiding this comment

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

  • the description texts should be foreground-level-6

Screenshot 2020-09-03 at 12 27 54

Screenshot 2020-09-03 at 12 39 25

  • need instead of enter in the header Screenshot 2020-09-03 at 12 29 35

  • additional whitespace after Radicle ID Screenshot 2020-09-03 at 12 32 21

  • keyboard shortcuts (eg search) should not work on the onboarding screens

@@ -0,0 +1,127 @@
<script>
Copy link
Member Author

@rudolfs rudolfs Sep 3, 2020

Choose a reason for hiding this comment

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

Note: adding this component only because Svelte didn't allow me to make the input type of <Input.Text> variable:
[svelte invalid-type] [E] 'type' attribute cannot be dynamic if input uses two-way binding.

@rudolfs
Copy link
Member Author

rudolfs commented Sep 3, 2020

If you have time in this PR, you could change the px to rem #303

6d2b297

Copy link
Contributor

@xla xla left a comment

Choose a reason for hiding this comment

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

Really like the structure and readability of the onboarding screen.

@@ -0,0 +1,127 @@
<script>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<script>
<script lang="ts">

Now that our tooling works for tsed components lets always use it for new ones.

ui/DesignSystem/Primitive/Input/Password.svelte Outdated Show resolved Hide resolved
@@ -0,0 +1,116 @@
<script>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<script>
<script lang="ts">

@@ -0,0 +1,172 @@
<script>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<script>
<script lang="ts">

@@ -0,0 +1,59 @@
<script>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<script>
<script lang="ts">

@@ -0,0 +1,53 @@
<script>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<script>
<script lang="ts">

@rudolfs
Copy link
Member Author

rudolfs commented Sep 4, 2020

Now that our tooling works for tsed components lets always use it for new ones.

xla: I wish I could, but that would mean I'd have to convert all the components that we use in those screens to TS as well. We should tackle that in a separate PR.

fullMessages: false,
};
$: beginValidation && validationStore.validate(handle);
$: allowNext = (handle && validationPasses()) || !validationStarted();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is validationStarted needed? If no handle is provided, it shouldn't allowNext and if one is provided, but validationPasses returns false it should not allowNext, ie if the validationStore.status is anything other than ValidationStatus.Success

Copy link
Member Author

Choose a reason for hiding this comment

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

If we don't have that check, then the button stays disabled forever.

Copy link
Contributor

Choose a reason for hiding this comment

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

hm ok, I also couldn't figure it out. Seems repetitive somehow

Copy link
Contributor

@xla xla left a comment

Choose a reason for hiding this comment

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

📮 🚧 ⛺️ 🚿

Copy link
Contributor

@MeBrei MeBrei left a comment

Choose a reason for hiding this comment

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

@rudolfs rudolfs merged commit bd9aed4 into master Sep 4, 2020
@rudolfs rudolfs deleted the rudolfs/onboarding-refresh branch September 4, 2020 14:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New onboarding
4 participants