add inline toolbar #3587
add inline toolbar #3587
Conversation
|
cc @mxstbr |
juliankrispel
force-pushed
the
inline-toolbar
branch
from
c7e76a5 to
ea8fbad
Compare
|
😍😍😍 |
|
@brianlovin let's get this on alpha? |
|
Can u please make sure I integrated it right? 😱
…On Tue, Jul 17, 2018 at 10:25 AM Max Stoiber ***@***.***> wrote:
@brianlovin <https://github.com/brianlovin> let's get this on alpha?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#3587 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABIhWonVEqCrBk3DNwmiKPm471BnOCxCks5uHZ98gaJpZM4VRiyp>
.
|
|
I‘m just a little paranoid I guess. Would be nice if someone else tested it
too 🙄
…On Tue, Jul 17, 2018 at 1:43 PM Max Stoiber ***@***.***> wrote:
***@***.**** approved this pull request.
You did!
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#3587 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABIhWqVqaTD1AhsKFNYyhMC9Vz1Mgh6wks5uHc3LgaJpZM4VRiyp>
.
|
|
@juliankrispel I tested it locally and it needs a z-index change and it's not implemented on our second composer (I know, we still maintain 2 separate composers which we need to unify). I'm happy to finish these items up, but might need direct access to the package in question. How should we go about publishing the inline editor package under Spectrum? cc @mxstbr |
|
To clarify on the composers: we have an "inbox composer" which opens when you click the compose icon from the 'home' tab. There's also a "profile composer" which opens when you start a conversation from a community/channel profile e.g. /figma |
|
@brianlovin @mxstbr sent you both invites, would probably be good to move this over to your org anyway ... |
|
Awesome! |
|
@brianlovin I'd be happy to make the updates meself - if you tell me exactly what they are |
|
@brianlovin @mxstbr ping |
|
Ah thought I replied here, sorry - will get back to this shortly @juliankrispel |
That's it! |
|
Thank you! Will get on that right away!
…On Wed, Jul 25, 2018 at 12:05 AM, Brian Lovin ***@***.***> wrote:
1. We - very painfully - maintain two composers right now. So we just
need to add this to both of them:
- src/components/composer
- and src/components/threadComposer
- the threadComposer appears on profile pages, like spectrum.chat/figma
1. I noticed a z-index issue when running this locally where it was
sitting behind the composer in the inbox view. It might be worthwhile to
just bump the zindex to something pretty high - somewhere between 2000
and 3000 looks to be good, according to src/components/globals/index
where we house our zindex variables.
That's it!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3587 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABIhWiVV3PZmsh3d3g-I3vRWzvy9T9W8ks5uJ6hMgaJpZM4VRiyp>
.
|
|
Ping 😊 |
|
Whoopsie! Sorry, my bad, got drowned in other stuff. So I think these are the two places you're talking off right @brianlovin ?
|
|
^ in which case we should be good to go, seems to work alright! |
|
@juliankrispel it's getting close, but now I'm running into a lot of bugs that we might not have noticed earlier. Here's a video: https://www.dropbox.com/s/9v4zw40sd8lw9x3/inline-editor.mp4?dl=0
This first one is the biggest pain point which is currently making the editor unusable. |
|
oh shit - it might be I screwed something up when I updated the dependency,
all that was working before.
I'm right on it!
…On Tue, Aug 7, 2018 at 5:24 PM, Brian Lovin ***@***.***> wrote:
@juliankrispel <https://github.com/juliankrispel> it's getting close, but
now I'm running into a lot of bugs that we might not have noticed earlier.
Here's a video:
https://www.dropbox.com/s/9v4zw40sd8lw9x3/inline-editor.mp4?dl=0
1. You can't click outside the toolbar to close it. This often results
in the toolbar getting stuck open
- This appears to only be happening in the inbox editor. The editor on
the community profile view *seems* to be working okay!
1. If you try to make text into a link, but don't insert a link, it
shouldn't create linked text - e.g. we should validate that the input
contains a url otherwise don't do anything
This first one is the biggest pain point which is currently making the
editor unusable.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3587 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABIhWthz2xqXjJvzNG5ti83BYuZsfLQrks5uOb88gaJpZM4VRiyp>
.
|
|
Thanks @juliankrispel! I suppose one additional thing before we wrap this all up too is: are we cool to migrate this inline repo to the Spectrum organization on GitHub and assign the npm project to @mxstbr ? |
|
absolutely, I'll do it asap!
…On Tue, Aug 7, 2018 at 5:28 PM, Brian Lovin ***@***.***> wrote:
Thanks @juliankrispel <https://github.com/juliankrispel>! I suppose one
additional thing before we wrap this all up too is: are we cool to migrate
this inline repo to the Spectrum organization on GitHub and assign the npm
project to @mxstbr <https://github.com/mxstbr> ?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3587 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABIhWqyTnhmS2hkQDwovGvJr3tAN0-HRks5uOcAagaJpZM4VRiyp>
.
|
|
sry folks been crazy busy, just tried to squeeze in some time to look at this today but I'm afraid it'll have to wait til the weekend ❤️ |
|
@juliankrispel I'm looking forward that feature!! |
|
Btw I've been debugging this on and off and have always run into issues 😢 . The refs are causing trouble when they're switched. I'll try my best to fix it, hopefully I'll get another evening tonight or this weekend! |
juliankrispel
force-pushed
the
inline-toolbar
branch
from
214f1d9 to
766ef8c
Compare
|
@brianlovin finally - fixed all the things! Please test once more, it should be good to go |
|
@brianlovin btw I tried transferring ownership of the spectrum inline toolbar repo to |
|
@juliankrispel gave this some more testing today and I'm only finding one lingering bug - but I think it will be pretty fast to fix. The checklist option:
has the following bug: if you select text, then choose the checkbox, you can't click to dismiss the toolbar anymore. I can't repro this with any of the other options. The reason I suggest this should be fast-to-fix is because we can remove that checklist option entirely from the toolbar. Right now it doesn't actually do anything and we don't have any supporting design/components to handle checkboxes anyways. After that, we're good to go as far as I can tell! cc @mxstbr any thoughts on how to get this transferred into our org? |
|
@brianlovin on it now! Let's get this done and dusted |
|
@brianlovin made the change and bumped the toolbar! No more checkboxes |
|
For some reason that circle ci test is now failing @brianlovin @mxstbr any idea? |
|
No longer relevant as we have moved our thread editor to plaintext + markdown: #4564 Thanks so much for your work on this anyway @juliankrispel! Sorry we never shipped it 🙏 |
|
❤️
…On Tue, Feb 12, 2019 at 11:18 AM Max Stoiber ***@***.***> wrote:
Closed #3587 <#3587>.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3587 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABIhWoZic-KrdFqbF4pWIPgOd1PW6eNxks5vMqLugaJpZM4VRiyp>
.
|
4 participants
Status
Deploy after merge (delete what needn't be deployed)
Run database migrations (delete if no migration was added)
NO
Release notes for users (delete if codebase-only change)