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

feat: add Advanced settings for offchain spaces. #789

Merged
merged 26 commits into from
Sep 24, 2024

Conversation

Sekhmet
Copy link
Member

@Sekhmet Sekhmet commented Sep 19, 2024

Summary

Adding advanced settings page for offchain spaces with following options:

  • Sub-spaces (parent and child spaces)
  • Terms of services
  • Custom domain
  • Hide from homepage
  • Delete space

Had to make some adjustments from the design, as not everything was clear:

  • Not extraneous modals for parent-space, sub-space, custom-domain (in designs we have input, but also modal that opens for you to type in, which I'm not sure makes sense, here I just use inputs directly avoiding modals.
  • Designs didn't cover how already added sub-spaces should look like, so I improvised.
  • No tooltip for custom domain input as we don't have it either on v1 so I didn't have text to use.

Closes: #

How to test

  1. Create new space (if you want to delete it, you will lose ability to recreate space under that domain).
  2. Go to your space, enter settings, you can modify advanced settings.
  3. You can delete your space.

Screenshots

localhost_8080_ (9)

Copy link

changeset-bot bot commented Sep 19, 2024

🦋 Changeset detected

Latest commit: f3c0791

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@snapshot-labs/sx Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

<UiContainerSettings
v-else-if="activeTab === 'advanced'"
title="Advanced"
description="Office ipsum you must be muted. Boy ocean define crank new."
Copy link
Member Author

Choose a reason for hiding this comment

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

@bonustrack will need text for this section.

Copy link
Member

@bonustrack bonustrack left a comment

Choose a reason for hiding this comment

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

It should be possible to add both a main space, and a sub spaces, it's not one or the other.

Also ideally we shouldn't allow duplicate for sub spaces.

apps/ui/src/components/FormSpaceAdvanced.vue Outdated Show resolved Hide resolved
Copy link
Contributor

@wa0x6e wa0x6e left a comment

Choose a reason for hiding this comment

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

Screenshot 2024-09-21 at 18 03 30

Disabled danger button should have disabled border color

apps/ui/src/components/Ui/Checkbox.vue Outdated Show resolved Hide resolved
Copy link
Contributor

@wa0x6e wa0x6e left a comment

Choose a reason for hiding this comment

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

The parent and child space input form should have a loading indicator

Screenshot 2024-09-21 at 18 24 54

Copy link
Contributor

@wa0x6e wa0x6e left a comment

Choose a reason for hiding this comment

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

Dirtying any fields in the new ADVANCED tab is not showing the bottom toolbar with SAVE button

Copy link
Contributor

@wa0x6e wa0x6e left a comment

Choose a reason for hiding this comment

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

Should prevent duplicate in child spaces

Screenshot 2024-09-21 at 18 30 22

apps/ui/src/composables/useActions.ts Outdated Show resolved Hide resolved
@wa0x6e
Copy link
Contributor

wa0x6e commented Sep 21, 2024

It should be possible to add both a main space, and a sub spaces, it's not one or the other.

Also ideally we shouldn't allow duplicate for sub spaces.

Seems like on v1, it's one or the other, both are not possible

@wa0x6e
Copy link
Contributor

wa0x6e commented Sep 21, 2024

Screenshot 2024-09-21 at 19 05 44

Space between checkbox and label seen insufficient

@Sekhmet Sekhmet force-pushed the sekhmet/advanced-settings branch from 0a4c0c1 to 12e2a85 Compare September 23, 2024 12:47
@Sekhmet
Copy link
Member Author

Sekhmet commented Sep 23, 2024

The parent and child space input form should have a loading indicator

Updated it to have similar space validation UX as v1.

Dirtying any fields in the new ADVANCED tab is not showing the bottom toolbar with SAVE button

Should be fixed now.

Space between checkbox and label seen insufficient

Followed designs there, but I agree this doesn't look good, increased it to gap-2.

@Sekhmet
Copy link
Member Author

Sekhmet commented Sep 23, 2024

It should be possible to add both a main space, and a sub spaces, it's not one or the other.

As Wan mentioned this replicates v1 UI that prevents configuring both parent and sub-spaces at the same time. I can remove that validation if we confirm that's what we want.

@Sekhmet Sekhmet requested a review from wa0x6e September 23, 2024 12:47
Copy link
Contributor

@wa0x6e wa0x6e left a comment

Choose a reason for hiding this comment

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

The domain name field is missing the length validation

Screenshot 2024-09-23 at 18 39 50

Putting something over the char limit does not show any errors.

And it's not on v1, but maybe it's time to also put a proper domain name validation on this field, since we'll be using it automatically for the whitelabel

On v1, there's a human manual validation, but we're moving away from that for v2

Copy link
Contributor

@wa0x6e wa0x6e left a comment

Choose a reason for hiding this comment

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

When putting an invalid space name in the parent space, it shows an error.

But it we change tab, and go back to advanced, the error state is gone.

There seems to be a query to check if the space is valid (loading icon on the input on tab change), but the error state is not being restored

Copy link
Contributor

@wa0x6e wa0x6e left a comment

Choose a reason for hiding this comment

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

Screenshot 2024-09-23 at 19 01 42

Shouldn't the message be different ?

@Sekhmet
Copy link
Member Author

Sekhmet commented Sep 23, 2024

When putting an invalid space name in the parent space, it shows an error.
But it we change tab, and go back to advanced, the error state is gone.
There seems to be a query to check if the space is valid (loading icon on the input on tab change), but the error state is not being restored

It's broken app-wide, we should fix it properly, but not really belongs in this PR, we can workaround it by using v-show which is not ideal (used it there for now).
#573

@Sekhmet
Copy link
Member Author

Sekhmet commented Sep 23, 2024

And it's not on v1, but maybe it's time to also put a proper domain name validation on this field, since we'll be using it automatically for the whitelabel

What you consider proper domain name validation? I think anything "proper" would be out of scope of this PR.

Copy link
Contributor

@wa0x6e wa0x6e left a comment

Choose a reason for hiding this comment

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

When reaching the max number of sub-spaces, the sub-space form is still accepting value, and validating space name.

But does not show any error when the space name is valid. I think it should show the error about reaching the max number of sub-spaces

@wa0x6e
Copy link
Contributor

wa0x6e commented Sep 23, 2024

out of scope of this PR.

For now, just alphanum, ., _ and - will cover 100% of our existing list (https://github.com/snapshot-labs/snapshot-spaces/blob/master/spaces/domains.json) we can optimize later

Copy link
Contributor

@wa0x6e wa0x6e left a comment

Choose a reason for hiding this comment

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

With some unsaved valid changes, navigating between non-advanced tab is keeping the bottom toolbar (with the save button) always shown.

But when navigating to advanced tab, the toolbar disappear for a spit-second, before reappearing

nevermind, can not reproduce anymore with the latest updates

Copy link
Member

@bonustrack bonustrack left a comment

Choose a reason for hiding this comment

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

tACK

@Sekhmet Sekhmet requested a review from wa0x6e September 24, 2024 05:04
Copy link
Contributor

@wa0x6e wa0x6e left a comment

Choose a reason for hiding this comment

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

Looks like the dirty state of the sub-spaces is only looking at the number of spaces.

Example:

  • My space already 3 sub-spaces
  • I remove 1 space -> SAVE button shows up
  • I add another different space -> SAVE button disappear

Copy link
Contributor

@wa0x6e wa0x6e left a comment

Choose a reason for hiding this comment

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

tAck

@Sekhmet Sekhmet merged commit aa87e0c into master Sep 24, 2024
3 checks passed
@Sekhmet Sekhmet deleted the sekhmet/advanced-settings branch September 24, 2024 07:42
@Sekhmet Sekhmet self-assigned this Sep 30, 2024
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.

3 participants