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

Show full colony name while creating colony #3130

Merged
merged 3 commits into from
Feb 10, 2022

Conversation

dit7ya
Copy link
Contributor

@dit7ya dit7ya commented Jan 25, 2022

Description

Shows full colony name instead of ENS name in the Create Colony Wizard.

Screenshot 2022-01-25 at 13-34-57 Colony
Screenshot 2022-01-25 at 13-35-10 Colony
Screenshot 2022-01-25 at 13-35-16 Colony

Note to reviewers

  • Was the ENS normalization useful for something I am totally missing?

Resolves #3128.

@dit7ya dit7ya added the bug label Jan 25, 2022
@dit7ya dit7ya requested a review from a team January 25, 2022 08:47
@dit7ya dit7ya self-assigned this Jan 25, 2022
Copy link
Member

@arrenv arrenv left a comment

Choose a reason for hiding this comment

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

@dit7ya looks much better, nice one.

What are your thoughts on the character limits? I feel that they could be increased to have a max of 2 lines. You could also add in a word-break: break-word; for long strings.

download (4)

download (5)

download (6)

Copy link
Contributor

@ArmandoGraterol ArmandoGraterol left a comment

Choose a reason for hiding this comment

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

Looks to be working as expected!

@arrenv raised a valid point, perhaps it could be beneficial to bring this up to Product to see what they think about increasing the limit, leaving it as it is, or breaking the words/sentence.

And about the normalization function, it seems like it comes from quite old code so I would consult with @rdig first to know what does it do, and if it's still needed before removing it

@dit7ya
Copy link
Contributor Author

dit7ya commented Jan 27, 2022

@arrenv @ArmandoGraterol I have no opinion on truncation length. However, I feel like if it is a very long name, there should be some visual indication (like bold font) for the colony name.

Adding word-break seems like a good idea irrespective of the above.

Copy link
Contributor

@alicjakujawa alicjakujawa left a comment

Choose a reason for hiding this comment

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

Are we sure that displayName will always exist? Shouldn't we fallback to colonyName?

@dit7ya
Copy link
Contributor Author

dit7ya commented Feb 9, 2022

@alicjakujawa I cannot think of a scenario of creating the colony without a full name. Does not the following make it required?
https://github.com/joincolony/colonyDapp/blob/f508fccad7f958707db356bb4262d560943401c7/src/modules/dashboard/components/CreateColonyWizard/StepColonyName.tsx#L82

If you want I can make it fallback to colonyName nonetheless.

@dit7ya dit7ya force-pushed the fix/3128-token-selector-colony-name branch from a59ea26 to 049f298 Compare February 9, 2022 09:16
@dit7ya
Copy link
Contributor Author

dit7ya commented Feb 9, 2022

Added word break and made 120 characters the limit for ellipsifying (chosen arbitrarily).

2022-02-09_14-21
2022-02-09_14-21_1
2022-02-09_14-22

Copy link
Member

@arrenv arrenv left a comment

Choose a reason for hiding this comment

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

@dit7ya It is looking good to me now.

long-colony-names-1

long-colony-names-2

long-colony-names-3

Copy link
Contributor

@chinins chinins left a comment

Choose a reason for hiding this comment

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

Good to go!

Copy link
Contributor

@ArmandoGraterol ArmandoGraterol left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@dit7ya dit7ya force-pushed the fix/3128-token-selector-colony-name branch from 049f298 to e95917d Compare February 10, 2022 17:24
@dit7ya dit7ya merged commit c4dcd24 into master Feb 10, 2022
@dit7ya dit7ya deleted the fix/3128-token-selector-colony-name branch February 10, 2022 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Token selector dialogs should show colony name instead of colony url
5 participants