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.

Sign up for GitHub

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

Jump to bottom

fix(markdown-slate): linked variables #367

Closed
wants to merge 1 commit into from
Closed

fix(markdown-slate): linked variables #367

wants to merge 1 commit into from

Conversation

d-e-v-esh
Copy link
Contributor

@d-e-v-esh d-e-v-esh commented Mar 23, 2021 •
edited by irmerk
Loading

Signed-off-by: d-e-v-esh [email protected]

Helps to resolve accordproject/web-components#175

The list items of the volumediscountolist template have linked variables. This is because they all are dealing with the same kind of variables. Checking of those variables are inside of a list and if that is true then changing their names fixes this issue.

Changes

  • Each line in the list comes as an object and encased under the type list_item.
  • They all have a key called children that contains an array with one element inside it.

chrome_FhfxaXRWCx

That element is another object with the type paragraph that contains the data for the actual data that is in the list item.

ZiCAagdCY1

It contains another array called children with the data that is inside the line like this.

chrome_kbQnAiimZi

As you can see, it has 7 different objects as elements of the array. Each element is there to create a divide between the normal text and variables and all the variables have a type variable

Each variable contains an object called data where the name of the variable resides

chrome_b8MvfbKAaf

We have to change that name and it works. We basically check if a variable is inside a list item and if that is true then we add the index of the list next to the variable and it looks like this.

chrome_auLJtPLeA9

Author Checklist

  • Ensure you provide a DCO sign-off for your commits using the --signoff option of git commit.
  • Vital features and changes captured in unit and/or integration tests
  • Commits messages follow AP format
  • Extend the documentation, if necessary
  • Merging to master from fork:branchname

@d-e-v-esh d-e-v-esh mentioned this pull request Mar 23, 2021
Open
@irmerk irmerk added Difficulty: Medium Type: Bug 🐛 Something isn't working labels Mar 25, 2021
@irmerk
Copy link
Member

irmerk commented Mar 25, 2021

Just a quick note @d-e-v-esh, this won't exactly close accordproject/web-components#175, because at the very least we'll want to release and update this package within web-components (assuming this PR does fix the bug).

@d-e-v-esh
Copy link
Contributor Author

@irmerk That is totally good. I can create another PR for web-components whenever needed.

@irmerk
Copy link
Member

irmerk commented Apr 2, 2021

Pinging @jeromesimeon and @dselman on this PR.

@jeromesimeon
Copy link
Member

jeromesimeon commented Apr 2, 2021 •
edited
Loading

hm...that doesn't seem right to me. If you change the variable names in Slate, you won't be able to convert Slate back to its original CiceroMark.

Also this issue isn't specific to lists. It will happen for any nested variable with the same name.

@jeromesimeon
Copy link
Member

Before you open a PR, it's useful to check that the linter and tests pass locally. You can do that by running npm run test in the root directory, or in the specific package you are changing.

@d-e-v-esh
Copy link
Contributor Author

hm...that doesn't seem right to me. If you change the variable names in Slate, you won't be able to convert Slate back to its original CiceroMark.

Also this issue isn't specific to lists. It will happen for any nested variable with the same name.

I don't know what would be the best way to solve this issue. This solution was proposed by Dan here. I followed through and it fixed the issue so I made the PR.

@d-e-v-esh
Copy link
Contributor Author

Before you open a PR, it's useful to check that the linter and tests pass locally. You can do that by running npm run test in the root directory, or in the specific package, you are changing.

Thanks for telling. I didn't know about that. I'll fix this PR and keep that in mind from now.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 88.949% when pulling 5c10920 on d-e-v-esh:linked-variables into e844a93 on accordproject:master.

@d-e-v-esh d-e-v-esh closed this by deleting the head repository Jan 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeromesimeon jeromesimeon Awaiting requested review from jeromesimeon

@dselman dselman Awaiting requested review from dselman

Assignees

@d-e-v-esh d-e-v-esh

Labels
Difficulty: Medium Type: Bug 🐛 Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

volumediscountolist template has linked variables
4 participants
@d-e-v-esh @irmerk @jeromesimeon @coveralls