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

feat(variables): add ability to add a variable to script from side menu #12706

Merged
merged 1 commit into from
Mar 19, 2019

Conversation

AlirieGray
Copy link
Contributor

@AlirieGray AlirieGray commented Mar 18, 2019

Closes #11699

This PR introduces the ability to add a variable to a script in the DE or VEO by clicking on a variable in the side menu of the script editor. This variable will be added where the user's cursor is located.

2019-03-18 15 35 14

  • Rebased/mergeable
  • Tests pass
  • CHANGELOG.md updated

@AlirieGray AlirieGray force-pushed the feat(variables)/add-to-script-from-side-menu branch from 7bd05fd to c49fcb2 Compare March 18, 2019 22:37
Copy link
Contributor

@ischolten ischolten left a comment

Choose a reason for hiding this comment

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

Looks great! I think the main thing that needs to get changed is the +2 that gets added when updating the cursor position. This feels like a very easy thing to break in the future.

@@ -29,7 +33,14 @@ const VariableItem: FunctionComponent<Props> = ({variable}) => {
onMouseLeave={() => setTooltipVisible(false)}
ref={trigger}
>
<div className="variables-toolbar--label">{variable.name}</div>
<div
Copy link
Contributor

Choose a reason for hiding this comment

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

could you make this its own component?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do!

const back = lineToEdit.slice(characterNumber, lineToEdit.length)

const updatedLine = `${front}v.${variableName}${back}`
const script = scriptLines
Copy link
Contributor

Choose a reason for hiding this comment

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

i dont think this line is necessary

const front = lineToEdit.slice(0, characterNumber)
const back = lineToEdit.slice(characterNumber, lineToEdit.length)

const updatedLine = `${front}v.${variableName}${back}`
Copy link
Contributor

Choose a reason for hiding this comment

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

could there be a function that takes the variable name and returns the namespaced variable as v.${varName}? This could then be used on line 134 to get the length to increase the ch by rather than the hardcoded 2 right now.


const updatedCursorPosition = {
line: currentLineNumber,
ch: currentCharacterNumber + variableName.length + 2,
Copy link
Contributor

Choose a reason for hiding this comment

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

this variableName.length + 2 seems like an arbitrary 2 which could possibly change. i have another comment about a potential solution to this.

variableName
)

const updatedCursorPosition = {
Copy link
Contributor

Choose a reason for hiding this comment

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

how do you feel about making a function for getting the cursor position? if the naming is a problem since theres already updateCursorPosition, there could potentially just be 2 files, one for inserting variables and one for inserting functions

@AlirieGray AlirieGray force-pushed the feat(variables)/add-to-script-from-side-menu branch 2 times, most recently from 09f63bf to ef696a8 Compare March 19, 2019 16:35
@AlirieGray AlirieGray force-pushed the feat(variables)/add-to-script-from-side-menu branch from ef696a8 to e8ffda2 Compare March 19, 2019 16:41
Copy link
Contributor

@ischolten ischolten left a comment

Choose a reason for hiding this comment

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

🍰

@AlirieGray AlirieGray merged commit 4309a70 into master Mar 19, 2019
@AlirieGray AlirieGray deleted the feat(variables)/add-to-script-from-side-menu branch March 19, 2019 16:53
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.

2 participants