Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Changing nick in the UI #551

Merged
merged 4 commits into from
Oct 1, 2016
Merged

Changing nick in the UI #551

merged 4 commits into from
Oct 1, 2016

Conversation

astorije
Copy link
Member

@astorije astorije commented Aug 9, 2016

This PR gives a nice UI integration for letting users change their nicks.

TODO (help needed)

  • R710 Since I'm actually modifying the content of the contenteditable box, I am currently leaving the saved value there, assuming that saving will just work. If the nick already exists, or if an invalid character is used, or if any other reason prevents IRC to change the nick, the label will keep the saved value until browsing among channels. 2 solutions:
    1. When saving, value of the label stays the saved value. If an error pops up, we rollback to the previous nick. This has the benefits of looking clean, but in practice it means we need to listen to the correct IRC errors, which might not be easy. For example, I recently noticed that a nick could not be saved, with a super generic, like "Failed action" or something like that. Not sure how irc-framework handles this, but it might just be not possible.
    2. ~~When saving, content of the label is reverted to previous nick. From there, we let the rest of the app changing the nick label as part of nick handling as it already does when sending /nick newnick. I actually prefer that solution! It looks less polished, but it's the smallest source of bugs. One caveat is that upon saving a new nick, users will see the old nick in a flash, then the new nick, but that can be easily handled by delaying the revert by a few seconds. I think I will go with this option unless I see a compelling argument not to.~~~
    3. Scratch all this, actual solution requires a mix of both and is rather complicated. I suggest going forward with this PR and improving upon this at a later time.
  • R1320 The whole nick box was being hidden in CSS when nick was empty with #form #nick:empty { visibility: hidden; }. Now that there are other elements in the box, #nick:empty will never be true. I am not sure how to fix this really. I could add some ifs to check when the connection is not established, or if there is an error or whatever, but the advantage of setting this with CSS was that we didn't need to bother about the why and just hide the item as a whole. Any smart idea?
  • Tooltips will look much nicer once Tooltips, tooltips everywhere #540 is merged.
  • Extra commits need to be squashed once everyone is happy with the implementation.

Result

set-nick

@astorije astorije added Type: Feature Tickets that describe a desired feature or PRs that add them to the project. help wanted labels Aug 9, 2016
@astorije astorije added this to the Next Release milestone Aug 9, 2016
@xPaw
Copy link
Member

xPaw commented Aug 14, 2016

Not a fan of persistent pencil icon, good implementation though.

@astorije
Copy link
Member Author

We can improve that later, but in the meantime it's better than nothing (and the icon can always be hidden in CSS :D). Not displaying it or just on hover or right click are way less-than-ideal solutions.

Also, I'm thinking about the same mechanism to solve #179.

@maxpoulin64, mind reviewing this (and #540 as well, which will improve this PR)? I'm open to advice regarding the second item, but worst case I'm thinking it's not a big deal and there is probably nothing to do there...

@williamboman
Copy link
Member

williamboman commented Aug 19, 2016

I agree with @xPaw. What about something like this;

#nick button#submit_nick,
#nick:not(.editable) #save-nick-tooltip,
#nick button#cancel_nick,
#nick:not(.editable) #cancel-nick-tooltip {
Copy link
Member Author

Choose a reason for hiding this comment

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

These should both use dashes, not underscores. Will update.

@astorije
Copy link
Member Author

@williamboman, hiding actions by default is not good in terms of usability (think discoverability) and accessibility (like conditions involving memory loss) so I'm 👎 to put this by default but 👍 to give the ability to do so.
On a browser, you would only find this by accident, while on a mobile, it's unlikely you'd find it at all.

As with other things, I suggest having it there (dimmed) by default, and giving ways to remove it if users don't like/want it, in the shape of custom CSS recipes:

  • Hide it completely (use /nick directly):

    #set_nick { display: none; }
  • Click on nick to edit:

    #nick { position: relative; }
    #set-nick-tooltip { width: 100%; position: absolute; left: 0; }
    #nick button#set_nick { width: 100%; }
    #nick button#set_nick:before { visibility: hidden; }

    (Or something similar, this was quickly hacked together)

Does that sound reasonable?

@astorije astorije force-pushed the astorije/set-nick-ui branch 3 times, most recently from bf00501 to d1be9aa Compare August 19, 2016 07:22
@astorije astorije force-pushed the astorije/set-nick-ui branch from d1be9aa to 3625d06 Compare September 2, 2016 04:23
@williamboman
Copy link
Member

hiding actions by default is not good in terms of usability (think discoverability)

What about having a shiny animation on it? Something like this but less... bad?

@astorije
Copy link
Member Author

What about having a shiny animation on it?

Like, always? I can wrap this in <blink></blink> maybe :D

@astorije
Copy link
Member Author

BTW, as said on IRC, the thing I'll do next as soon as this makes it into master is to have a way to cycle through nicks on mobile. Posting it here because I know this PR is going to mess with @maxpoulin64's hack.

@williamboman
Copy link
Member

Like, always?

In my head I'm picturing an effect similar to the one I posted, but the animation goes diagonally from bottom left to top right once every 10s.

I can wrap this in maybe :D

facepalm

@MaxLeiter
Copy link
Member

MaxLeiter commented Sep 21, 2016

I'm personally fine with the pencil; I agree with @astorije that having a 'hidden' feature like this could lead to a lot of confusion (especially on mobile).

@zachbr
Copy link

zachbr commented Sep 21, 2016

I was initially against a pencil showing all the time because I thought it was a tad verbose, I still do to an extent, however it does massively improve discoverability, and it looks like it'll be easy to hide if that's the user's intent 👍

@astorije astorije force-pushed the astorije/set-nick-ui branch from 3625d06 to 96b2835 Compare September 30, 2016 05:45
@astorije
Copy link
Member Author

astorije commented Sep 30, 2016

Any chance this can be reviewed and merge to land in 2.1.0? We can improve UI afterwards, but the logic is there and ready :-)

(Rebased with master and fix conflicts)

@xPaw xPaw force-pushed the astorije/set-nick-ui branch from 96b2835 to 1d40ba4 Compare October 1, 2016 17:04
@xPaw xPaw self-assigned this Oct 1, 2016
@xPaw
Copy link
Member

xPaw commented Oct 1, 2016

The PR is 👍 but I fixed a couple of things. Merge it when you review them.

The whole nick box was being hidden in CSS when nick was empty with #form #nick:empty { visibility: hidden; }

I don't think that rule is even needed anymore.

@astorije astorije force-pushed the astorije/set-nick-ui branch from 1d40ba4 to 024369d Compare October 1, 2016 20:51
@astorije
Copy link
Member Author

astorije commented Oct 1, 2016

I don't think that rule is even needed anymore.

Indeed, thanks to your last commit. Awesome set of commits btw, thanks for these! 👍

@astorije astorije modified the milestones: 2.1.0, Next Release Oct 1, 2016
@astorije astorije self-assigned this Oct 1, 2016
@astorije astorije merged commit cd87df9 into master Oct 1, 2016
@astorije astorije deleted the astorije/set-nick-ui branch October 1, 2016 20:59
matburnham pushed a commit to matburnham/lounge that referenced this pull request Sep 6, 2017
@xPaw xPaw unassigned astorije and xPaw Mar 12, 2018
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Type: Feature Tickets that describe a desired feature or PRs that add them to the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants