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

T3chguy/devtools #4735

Merged
merged 15 commits into from
Sep 17, 2017
Merged

T3chguy/devtools #4735

merged 15 commits into from
Sep 17, 2017

Conversation

t3chguy
Copy link
Member

@t3chguy t3chguy commented Aug 3, 2017

Add 3 "Developer Tools"

  • Send Custom Event
  • Send Custom State Event
  • Room State Browser

@t3chguy
Copy link
Member Author

t3chguy commented Aug 9, 2017

Signed-off-by: Michael Telatynski [email protected]

@lukebarnard1 lukebarnard1 self-assigned this Aug 10, 2017
@lukebarnard1
Copy link
Contributor

A few suggestions:

  • the "Event Content" box should be as wide as the inputs above and auto-expand to fit the content
  • there should be a back button at all times when in devtools:
    • when viewing the source of an event
    • when sending an event or state event

@t3chguy
Copy link
Member Author

t3chguy commented Aug 10, 2017

thrown a back button everywhere

Just need to figure out how to make it look nicer and to make the textarea autoresize

@t3chguy
Copy link
Member Author

t3chguy commented Aug 10, 2017

@lukebarnard1 could you have another play pl0x
Autoresizing doesn't seem like fun without a dep
I know i18n is missing for now, I'll do it if you're happy with it otherwise

Copy link
Contributor

@lukebarnard1 lukebarnard1 left a comment

Choose a reason for hiding this comment

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

This is looking really good :)

await this.send(content);
message = _t('Event sent!');
} catch (e) {
message = `${_t('Failed to send custom event.')} (${e.toString()})`;
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest not using `` here because it's hard to read.

<label htmlFor="evContent"> { _t('Event Content') } </label>
</div>
<div>
<textarea id="evContent" ref="evContent" className="mx_TextInputDialog_input" defaultValue={"{\n\n}"} cols="63" rows="5" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably OK to not use {} in defaultValue. As in defaultValue="..."

Copy link
Member Author

Choose a reason for hiding this comment

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

It's required for literal new lines

Copy link
Contributor

Choose a reason for hiding this comment

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

ah fair enough

} else {
body = <div>
<div className="mx_Dialog_content">
<button onClick={this._setMode(SendCustomEvent)}>{ _t('Send Custom Event') }</button>
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose we should be doing the whole skinning dance here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Skinning dance?

Copy link
Contributor

Choose a reason for hiding this comment

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

Using the component skinning that we support. i.e. sdk.getComponent('bla')

Copy link
Member Author

Choose a reason for hiding this comment

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

oh so you're suggesting that I pull in the 3 tools through the sdk? ooi why is that done if they're defined at the riot-web level which is designed to be the top level?

this.refs.stateKey.value);
}

render() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Given the only difference between this render and the render being clobbered, I think you should have this._content() that can be clobbered to return the correct form for the given component. This keeps the render the same.

}

onBack() {
if (this.state.event) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ordinarily I'd vote against this but given that the setStates are here, it's fairly clear what will happen when clicking "Back" several times. The alternative would be doing some routing and I don't think that's a reasonable consideration for /devtools.

Copy link
Contributor

@lukebarnard1 lukebarnard1 left a comment

Choose a reason for hiding this comment

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

see changes requested above


_buttons() {
return <div className="mx_Dialog_buttons">
<button onClick={this.onBack}>{ _t('Back') }</button>
Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be great for these buttons to have margin-bottom: 10px or whatever.

Copy link
Member Author

Choose a reason for hiding this comment

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

like so: ?
image

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, no, I meant on the event browser

Copy link
Member Author

Choose a reason for hiding this comment

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

to stick the buttons to the modal and only scroll the event?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@lukebarnard1 lukebarnard1 assigned t3chguy and unassigned lukebarnard1 Aug 30, 2017
@t3chguy
Copy link
Member Author

t3chguy commented Aug 31, 2017

@lukebarnard1 PTAL

@ara4n
Copy link
Member

ara4n commented Sep 17, 2017

@t3chguy: this is awesome and i had no idea it existed - thanks! @lukebarnard1: i assume it's just an oversight that you approved but didn't merge! :)

@ara4n ara4n merged commit 828e9d5 into develop Sep 17, 2017
@t3chguy t3chguy deleted the t3chguy/devtools branch October 29, 2017 17:27
@t3chguy t3chguy restored the t3chguy/devtools branch October 29, 2017 17:27
@t3chguy t3chguy deleted the t3chguy/devtools branch March 23, 2022 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants