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

move everything not explicitly riot (or status) branded into matrix-react-sdk #6500

Merged
merged 22 commits into from Apr 18, 2018

Conversation

@ara4n
Copy link
Member

ara4n commented Apr 11, 2018

This moves almost everything not explicitly riot (or status) branded into matrix-react-sdk, in order to fix the layering of riot-web v. matrix-react-sdk, as per https://docs.google.com/document/d/1itOIy6zdKGJCQZEPecL9uTPTtBCVrZ_HgwQiDnah_LM.

For now we're deliberately keeping the vector/ directory (i.e. the top level app skin itself) in the riot-web repo. In future we might want to provide a simple sample skin app in matrix-react-sdk.

Status:

  • this moves the files into the right(?) place
  • actually make it build again.
  • fix the tests
  • remerge the i18n

Whilst this PR is in flight, I suggest folks try to avoid making changes to the riot-web repository.

Twinned with matrix-org/matrix-react-sdk#1836

@martindale

This comment has been minimized.

Copy link

martindale commented Apr 13, 2018

Recommend: prefix in-flight PRs with WIP: in their title. This looks awesome, otherwise!

@ara4n

This comment has been minimized.

Copy link
Member Author

ara4n commented Apr 16, 2018

Right, I think this is good to go now.

@martindale yup - mea culpa on missing the WIP, although events have now overtaken that :)

Copy link
Contributor

lukebarnard1 left a comment

Just minor things, otherwise looks good.

@@ -0,0 +1,2 @@
Any UTs for vector-web layer components or functionality should go here.
This used to contain the UTs for notifications before they got moved to react-sdk.

This comment has been minimized.

Copy link
@lukebarnard1

lukebarnard1 Apr 17, 2018

Contributor

Commit message is misleading here. Should there be changes to the tests as part of this commit?

This comment has been minimized.

Copy link
@ara4n

ara4n Apr 17, 2018

Author Member

sorry, this was a terrible commit message. the thing that was breaking the tests is that the unit-tests dir had winked out of existence. creating this README file recreated the directory and fixed the tests O:-)

displayName: 'HomePage',
displayName: 'VectorHomePage',
statics: {
replaces: 'HomePage',

This comment has been minimized.

Copy link
@lukebarnard1

lukebarnard1 Apr 17, 2018

Contributor

This looks like it needs a comment.

This comment has been minimized.

Copy link
@ara4n

ara4n Apr 17, 2018

Author Member

i forgot about this. I need to turn HomePage into a ES6 class and then have VectorHomePage inherit from it (whilst also replaceing it)

};
},
class VectorHomePage extends HomePage {
displayName = 'VectorHomePage'

This comment has been minimized.

Copy link
@dbkr

dbkr Apr 18, 2018

Member

Should this have a 'static' too? Also I think these want semicolons.

require('matrix-react-sdk/node_modules/gemini-scrollbar/gemini-scrollbar.css');
require('matrix-react-sdk/node_modules/gfm.css/gfm.css');
require('matrix-react-sdk/node_modules/highlight.js/styles/github.css');
require('matrix-react-sdk/node_modules/draft-js/dist/Draft.css');

This comment has been minimized.

Copy link
@dbkr

dbkr Apr 18, 2018

Member

I assume this didn't work without hardcoding the path? I think this might break between versions of npm.

This comment has been minimized.

Copy link
@ara4n

ara4n Apr 18, 2018

Author Member

yup, doesn't work if you try matrix-react-sdk/library-name/whatever.css. we have the same problem when doing SCSS imports of CSS from react-sdk into the vector layer. Any idea how we can make it non-npm-specific?

This comment has been minimized.

Copy link
@dbkr

dbkr Apr 18, 2018

Member

oh yep, I thought I'd seen that somewhere else. I don't know off hand any other way to do this, I guess other than importing the dependency directly.

@dbkr
dbkr approved these changes Apr 18, 2018
@ara4n ara4n merged commit ddc20e2 into develop Apr 18, 2018
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.