Skip to content
This repository was archived by the owner on Oct 11, 2022. It is now read-only.

Conversation

@ThomasRoest
Copy link
Contributor

@ThomasRoest ThomasRoest commented Jan 24, 2019

  • WIP
  • Ready for review
  • Needs testing

Deploy after merge (delete what needn't be deployed)

  • hyperion (frontend)

Related issues (delete if you don't know of any)
#3071
#4564

This is where I'm right now, refactoring the threadcomposer ( combining two composers). Still quite a few things to fix, including

TODO

  • switch to different composer component, same as in Move thread editor to plaintext #4564
  • add new composer + toggle to community view
  • add new composer + toggle to inbox view
  • add new composer to channel view
  • tests / flowtypes

@spectrum-bot
Copy link

spectrum-bot bot commented Jan 24, 2019

Warnings
⚠️

These modified files do not have Flow enabled:

  • src/components/composer/style.js
  • src/components/globals/index.js
  • src/components/threadComposer/index.js
  • src/components/threadComposer/style.js

Generated by 🚫 dangerJS

@ThomasRoest ThomasRoest changed the title WIP: threadcomposer refactor first iteration WIP: threadcomposer refactor Jan 24, 2019
@mxstbr mxstbr mentioned this pull request Jan 24, 2019
25 tasks
@ThomasRoest
Copy link
Contributor Author

ThomasRoest commented Jan 25, 2019

current status (with inserter)

screen recording 2019-01-25 at 03 52 pm

@ThomasRoest
Copy link
Contributor Author

image 2019-01-28 at 10 46 43 am

@brianlovin
Copy link
Contributor

Coming along super nicely!

@ThomasRoest ThomasRoest force-pushed the feature/refactor-threadcomposer branch from ba288c2 to af355be Compare January 31, 2019 11:49
@ThomasRoest
Copy link
Contributor Author

ThomasRoest commented Feb 3, 2019

Before I continue with this, I would like to know what you guys think of this.
One thing I noticed is that @mxstbr is now working on the composer in src/components/composer/index.js This is the composer you see on mobile and when you visit /new/thread

While I'm working on the composer in

src/components/threadComposer/components/composer.js

It might be good to wait for #4564 to be completed before continuing?

@mxstbr
Copy link
Contributor

mxstbr commented Feb 4, 2019

We should for sure be using the same one—I totally forgot we had two! 🤦‍♂️

@ThomasRoest ThomasRoest changed the title WIP: threadcomposer refactor WIP: combine / refactor threadcomposer Feb 5, 2019
@ThomasRoest
Copy link
Contributor Author

ThomasRoest commented Feb 5, 2019

@mxstbr ok, I'll see if I can change this and I'll start working in src/components/composer/index.js

@ThomasRoest
Copy link
Contributor Author

Wasn't that much trouble to switch components in the slider, so now we should be working on the same component:)

screen recording 2019-02-05 at 01 17 pm

@mxstbr
Copy link
Contributor

mxstbr commented Feb 5, 2019

Amazing!!! 🎉 🎉 🎉 You are the best, that should make the merge from #4564 to this much much easier. Do you want to do that in this PR, is #4564 blocking you?

@ThomasRoest
Copy link
Contributor Author

not sure yet, I still have to fix a few things before we can merge

@mxstbr
Copy link
Contributor

mxstbr commented Feb 7, 2019

@ThomasRoest given that you combined the two composer, I think we will have to merge this into #4564 and ship both at the same time. Otherwise the composer in the inbox and on /new/thread will be plaintext, but the composer on the community and channel views will be WYSIWYG—not ideal! 😅


Do you have any more changes locally? If so, can you push them so I can merge this into #4564 and we can continue working on that PR?!

Amazing work so far, excited about this now-entire overhaul of the composer! Folks are going to be very excited! 🎉

@ThomasRoest
Copy link
Contributor Author

yes I still have some changes locally! I will push them asap

@mxstbr
Copy link
Contributor

mxstbr commented Feb 7, 2019

Thank you! 💯

@ThomasRoest
Copy link
Contributor Author

ThomasRoest commented Feb 8, 2019

This needs more work, but you can take it from here if you want.

Basically what I've done is adding some styles depending on where the composer is opened. So the same component is used in /new/thread, community, inbox and channel.

Some things todo

  • move/refactor placeholder and upsell component from components/threadComposer/components/placeholder.js and check if upsell works as it should.
  • check if activethread and community are set when opening the composer ( in the select inputs )
    At the moment, this does not work correctly when you open the composer in a channel within a community
  • add/fix some tests and flowtypes

update

  • fix problem with z-index in safari

@mxstbr
Copy link
Contributor

mxstbr commented Feb 8, 2019

Amazing, I will dig into this today!

flex: auto;
overflow: hidden;
height: calc(100vh - 48px);
height: 100vh;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should not change, otherwise the thread composer overflows on /new/thread! (48px is the navbar height)

Suggested change
height: 100vh;
height: calc(100vh - 48px);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh I missed that, probably an idea to only add those styles with the isSlider props, similar to other styles

@mxstbr
Copy link
Contributor

mxstbr commented Feb 8, 2019

I have merged this into #4564, let's continue collaborating with PRs against that branch! Thanks so much for your work here @ThomasRoest, this is awesome 💯

@mxstbr mxstbr closed this Feb 8, 2019
@brianlovin
Copy link
Contributor

Thank you for the work here @ThomasRoest! Can't wait to get this better experience live

@brianlovin brianlovin mentioned this pull request Feb 22, 2019
1 task
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants