move everything not explicitly riot (or status) branded into matrix-react-sdk#1836
Conversation
…a field rather than export
ara4n
force-pushed
the
matthew/fix_layering
branch
from
550eac1 to
0336d99
Compare
|
Right, I think this is good to go now. @lukebarnard1 i think i nominated you to do a sanitycheck? O:-) |
lukebarnard1
left a comment
lukebarnard1
left a comment
There was a problem hiding this comment.
Just a few minor problems with the changes themselves. Apart from those, I noticed:
- Missing language files, such as ta.json (presumably because the scripting done for this assumed ta.json and others would already exist in matrix-react-sdk).
- Missing images pN.png
| this.props.onFinished(false); | ||
| const QuestionDialog = sdk.getComponent("dialogs.QuestionDialog"); | ||
| Modal.createTrackedDialog('Bug report sent', '', QuestionDialog, { | ||
| Modal.createTrackedDialog(_t('Bug report sent'), '', QuestionDialog, { |
There was a problem hiding this comment.
This is unnecessary, that string is only used by the analytics.
| export default React.createClass({ | ||
| propTypes: { | ||
| status: React.PropTypes.oneOf(Object.values(updateCheckStatusEnum)).isRequired, | ||
| status: React.PropTypes.string, // oneOf(Object.values(updateCheckStatusEnum)).isRequired, |
There was a problem hiding this comment.
This should probably still be marked as isRequired and have an explanation the values it should have. Alternatively, there doesn't seem to be anything from stopping updateCheckStatusEnum from being called from outside of the class (VectorBasePlatform returns a constant and none of the individual platforms override getUpdateCheckStatusEnum)
| }, | ||
|
|
||
| getStatusText: function() { | ||
| const updateCheckStatusEnum = PlatformPeg.get().getUpdateCheckStatusEnum(); |
There was a problem hiding this comment.
Leaving a comment to explain why this is being done would be very useful.
| @@ -1,2 +1,2 @@ | |||
| @import "_base.scss"; | |||
| @import "../_components.scss"; No newline at end of file | |||
| @import "../../../../res/css/_components.scss"; No newline at end of file | |||
| @import "../../light/css/_base.scss"; | ||
| @import "_dark.scss"; | ||
| @import "../_components.scss"; | ||
| @import "../../../../res/css/_components.scss"; No newline at end of file |
| @mixin mx_DialogButton; | ||
| font-size: 15px; | ||
| padding: 0px 1.5em 0px 1.5em; | ||
| } |
There was a problem hiding this comment.
Kind of annoying that this ended up in a different commit, otherwise the net change is moving _base.scss
There was a problem hiding this comment.
i'm a bit unsure whether _base.scss should be hidden in light (as in practice the default base.scss is the light theme) or somewhere more common. i'm not sure it makes much difference really so i'm leaving it here for now.
|
|
||
| find . \! \( -path ./themes -prune \) -iname _\*.scss | | ||
| fgrep -v _components.scss | LC_ALL=C sort | | ||
| find . -iname _\*.scss | fgrep -v _components.scss | LC_ALL=C sort | |
There was a problem hiding this comment.
Explaining the change here in the commit/in a comment would be nice, but this seems to have worked so LGTM.
There was a problem hiding this comment.
have commented for posterity in the code.
oooops - yup, turns out copy-i18n doesn't work if there's not a target to copy into. excellent catch; thanks. the missing image files are deliberately dropped as they haven't been used since vector 0.5 or so. otherwise, I think i've incorporated all of the review - @lukebarnard1, thanks! all that remains is to fix the VectorHomePage mess. |
|
lgtm |
3 participants
This is the other half of element-hq/element-web#6500; moving all the non-riot/status-branded UI from riot-web into matrix-react-sdk as per https://docs.google.com/document/d/1itOIy6zdKGJCQZEPecL9uTPTtBCVrZ_HgwQiDnah_LM.
Status:
Whilst this PR is in flight, I suggest folks try to avoid making changes to the riot-web repository.