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

Add StoredVars tab #220

Open
wants to merge 122 commits into
base: trunk
Choose a base branch
from
Open

Add StoredVars tab #220

wants to merge 122 commits into from

Conversation

Jongkeun
Copy link
Contributor

I need the storedVar tab before reference tab.

@corevo corevo mentioned this pull request Aug 20, 2018
@Jongkeun
Copy link
Contributor Author

I modified that you mentioned.
Check it, please.

Copy link
Member

@corevo corevo left a comment

Choose a reason for hiding this comment

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

Some general feedback:

  • I have to click twice to change between the variable name to it's value, once to blur the previous input, and another to focus the next, instead of simply moving between them.
  • Hitting tab won't switch between the variable name and value inputs.
  • Hitting tab also locks you inside the variable editing and won't let you reach other elements.

<strong className="value">Value</strong>
<div className="deleteBtn"/>
</li>}
{variables.storedVars.entries().sort().map((storedMap) => (
Copy link
Member

Choose a reason for hiding this comment

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

Keep them sorted via a computed property, either in the store, or in the component.

</li>}
{variables.storedVars.entries().sort().map((storedMap) => (
<Variable
key={Math.random()}
Copy link
Member

Choose a reason for hiding this comment

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

This beats the purpose of having a key in the first place, the idea is to have a persistent key throughout renders.
You can use the variable name as a key, since it is unique.

@@ -36,6 +37,10 @@ class Variables {
@action.bound clearVariables() {
this.storedVars.clear();
}

@computed get isStop(){
Copy link
Member

Choose a reason for hiding this comment

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

This is component specific logic, and has nothing to do with the general behavior of the variables store.

return (
<li className="variable">
{this.props.isAdding ?
<input
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason to use the default input element, and not the Input component?

this.input.focus();
}
handleKeyDown(e) {
if (e.key === "Enter") {
Copy link
Member

Choose a reason for hiding this comment

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

Use a form's onSubmit event, instead of keydown and checking for the key enter.
It is bypassing the browser default behavior.

}
}
handleChanged() {
const isValidKey = this.state.key || this.props.keyVar;
Copy link
Member

Choose a reason for hiding this comment

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

The naming convention is misleading, variables that begin with is are booleans, while these are strings.
Perhaps call them validKey?

}
handleChanged() {
const isValidKey = this.state.key || this.props.keyVar;
const isValidValue = this.state.value || this.props.value;
Copy link
Member

Choose a reason for hiding this comment

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

Why is empty string considered invalid? users can definitely store an empty string.

}
addVariableClicked(isAdding) {
this.setState({
isAddingVariable: isAdding
Copy link
Member

Choose a reason for hiding this comment

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

I don't like this state, it is also inconsistent with the experience we give with the command table,
let's just have a "pristine" variable line, just like in the command table.

@Jongkeun
Copy link
Contributor Author

@corevo Could you check it out?

@corevo
Copy link
Member

corevo commented Nov 4, 2018

I was sick the past few weeks, I'll make sure to have a look.

@corevo
Copy link
Member

corevo commented Dec 3, 2018

Hey, I've given this some thought, and theres a design issue we should discuss, wanna ping my on irc, or something.
I'll make sure to relay our decision over to this thread.

@Jongkeun
Copy link
Contributor Author

Jongkeun commented Dec 4, 2018

I don't care what happens to this pr, even though this pr was deleted.
This pr is in your hands. I will just follow your think.
Let me know if there's anything I need to do.

@corevo
Copy link
Member

corevo commented Dec 4, 2018

Do you mind if I take ownership?
The problem is the fact that everything gets converted to strings, while you can potentially store and JSON serializable object, plus 2 types (WebElement and WindowHandle).
We need to make sure that if a user edits an integer it won't get converted into a string, and so forth.

@Jongkeun
Copy link
Contributor Author

Jongkeun commented Dec 4, 2018

@corevo Sure, why not?
Please make this a good program.

@corevo
Copy link
Member

corevo commented Dec 4, 2018

👍

@cdifino
Copy link

cdifino commented Jul 30, 2019

Hi @corevo ! I would like to know what happened to this PR? What is the status? and is someone working on it? I think it is a great feature to be implemented in the extension.

@corevo
Copy link
Member

corevo commented Aug 1, 2019

@cdifino the problem is typings, since execute script may return for example an array, this variables tab will cast it to string.

We need to support specific forms for specific variable types as to avoid casting them to strings.

@CLAassistant
Copy link

CLAassistant commented Nov 23, 2019

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

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.

4 participants