Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Update Slate (#7419) #2186

Closed

Conversation

aaronraimist
Copy link
Contributor

@aaronraimist aaronraimist commented Oct 1, 2018

Fixes element-hq/element-web#7419

Issue was fixed by using a more recent version of Slate. ianstormtaylor/slate#982

Tests pass and everything appears to work with the latest versions of slate and slate-react so I went ahead and just used the latest version.


Unrelated but I was somewhat confused as CONTRIBUTING.rst says matrix-react-sdk follows the same pattern as Synapse but there is no Towncrier in here as far as I can tell.

Also why is the webpack version in here so old? I had to install 4.0.0 manually.

@turt2live
Copy link
Member

I'm starting to think these PRs are for Hacktoberfest ;) Thanks for the PR anyhow, and here's hoping you'll be one of the 50k to get a tshirt.

I'm a bit worried that an older version of slate was used for some other reason. The end-to-end testing appears to have also caught an issue with the composer itself: https://travis-ci.org/matrix-org/matrix-react-sdk/builds/435783790#L2355 (luckily most of the console spam will be fixed by #2188).

As for the contributing.md: good spot, I've opened element-hq/element-web#7430 to hopefully get it addressed. I'm not sure if we should copy/paste it into this repo or maybe spin up a repo specifically for holding the generic document, but suggestions here are appreciated (what would be easier for you, as a contributor?)

The old webpack is an interesting one. I don't see anything in the package.json that would use it, so it might have been included in error. @ara4n or @dbkr - do either of you happen to know what's going on here?

@t3chguy
Copy link
Member

t3chguy commented Oct 2, 2018

Older slate was pinned due to a bug they had I have since fixed upstream

@dbkr
Copy link
Member

dbkr commented Oct 2, 2018

It's possible this will also fix element-hq/element-web#7105

If the bug is now fixed upstream and this works then we should update, although last time I tried to install the latest slate, everything broke (I forget what version though). @ara4n wdyt?

I suspect we only have webpack as a dep in here because of the unit tests. We updated to webpack 4 in riot-web the other day, we should update here too (and look at whether we actually need to depend on it directly).

@aaronraimist
Copy link
Contributor Author

The end-to-end testing appears to have also caught an issue with the composer itself: https://travis-ci.org/matrix-org/matrix-react-sdk/builds/435783790#L2355

How should I go about debugging what is wrong with the composer?


I'm starting to think these PRs are for Hacktoberfest ;) Thanks for the PR anyhow, and here's hoping you'll be one of the 50k to get a tshirt.

Hacktoberfest? Actually I think it's just a coincidence that I started working on these in October.

@turt2live
Copy link
Member

I'm not sure where to start debugging them to be honest. Did you see the placeholder while testing? @bwindels might have ideas (although he's on holiday at the moment).

As for hacktoberfest: it's basically a thing where if you open 5 PRs in October you can get a t-shirt: https://hacktoberfest.digitalocean.com/ - it's to promote open source and have a more thriving community. Looks like your timing was just perfect given you got 5 PRs on the 1st of the month :p
I'm sure if you sign up now it'll back-credit the PRs for you, if you're interested.

@aaronraimist
Copy link
Contributor Author

No I didn't see "Send a message (unencrypted)" pop up at the wrong time when I was testing it but admittedly I didn't play with it for that long. Will test more tomorrow.

@dbkr
Copy link
Member

dbkr commented Oct 5, 2018

Did you try autocomplete or emoji with this? Just tested with these versions and typing an emoji gives ``Node.assertDescendant could not find node with path or key: null and autocomplete just doesn't appear at all.

@bwindels
Copy link
Contributor

bwindels commented Oct 8, 2018

Yeah, the e2e tests seem to break on typing in the composer. The way to start debugging them would be to install them locally and run them in windowed mode and dev tools open. I'll try to have a look as well.

@dbkr
Copy link
Member

dbkr commented Oct 9, 2018

I've done the necessary API changes in #2202 (turns out there were quite a few). Thanks for looking at this!

@dbkr dbkr closed this Oct 9, 2018
@aaronraimist aaronraimist deleted the update-slate-7419 branch October 23, 2018 02:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[OSX][QWERTY] Emphasized character preceded by the unaccented character
5 participants