-
-
Notifications
You must be signed in to change notification settings - Fork 833
move everything not explicitly riot (or status) branded into matrix-react-sdk #1836
Conversation
…a field rather than export
550eac1
to
0336d99
Compare
Right, I think this is good to go now. @lukebarnard1 i think i nominated you to do a sanitycheck? O:-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -65,7 +65,7 @@ export default class BugReportDialog extends React.Component { | |||
if (!this._unmounted) { | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unnecessary, that string is only used by the analytics.
@@ -23,7 +23,7 @@ import AccessibleButton from '../../../components/views/elements/AccessibleButto | |||
|
|||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
)
@@ -42,6 +36,7 @@ export default React.createClass({ | |||
}, | |||
|
|||
getStatusText: function() { | |||
const updateCheckStatusEnum = PlatformPeg.get().getUpdateCheckStatusEnum(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leaving a comment to explain why this is being done would be very useful.
res/themes/light/css/light.scss
Outdated
@import "../../../../res/css/_components.scss"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: no NL at EOF
res/themes/dark/css/dark.scss
Outdated
@import "_dark.scss"; | ||
@import "../_components.scss"; | ||
@import "../../../../res/css/_components.scss"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: no NL at EOF
@mixin mx_DialogButton; | ||
font-size: 15px; | ||
padding: 0px 1.5em 0px 1.5em; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
res/css/rethemendex.sh
Outdated
@@ -5,8 +5,7 @@ cd `dirname $0` | |||
{ | |||
echo "// autogenerated by rethemendex.sh" | |||
|
|||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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.