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

issue/2876 JSX version of text #57

Merged
merged 8 commits into from
Aug 26, 2021
Merged

issue/2876 JSX version of text #57

merged 8 commits into from
Aug 26, 2021

Conversation

ghost
Copy link

@ghost ghost commented Aug 12, 2020

@ghost ghost requested a review from oliverfoster August 12, 2020 16:44
@ghost
Copy link
Author

ghost commented Aug 13, 2020

Found a framework bug during the conversion: when instruction text isn't provided, components.jsx line 42 attempts to evaluate mobileInstruction but mobileInstruction isn't defined in components.jsx lines 7-13

  const {
    displayTitle,
    body,
    instruction,
    _component,
    _disableAccessibilityState
  } = data;

because, presumably, it isn't defined on the component model. This results in runtime error 'Uncaught ReferenceError: mobileInstruction is not defined'.

Update: This also occurs when IT is left off the accordion (only other component I currently have using jsx).

@oliverfoster
Copy link
Member

oliverfoster commented Aug 13, 2020

Fixed in commit adaptlearning/adapt_framework@87f15ed hopefully.

because, presumably, it isn't defined on the component model. This results in runtime error 'Uncaught ReferenceError: mobileInstruction is not defined'.

It's because this line https://github.com/adaptlearning/adapt_framework/blob/issue/2824-jsx/src/core/templates/partials/component.jsx#L43 is using a variable which is hasn't been defined, only if instruction is a falsie. I forgot to declare that variable from the data on the lines you've referenced https://github.com/adaptlearning/adapt_framework/blob/issue/2824-jsx/src/core/templates/partials/component.jsx#L7-L14

@oliverfoster
Copy link
Member

oliverfoster commented Aug 13, 2020

As discussed, things I need to work on:

  • core/templates/partial/component.jsx needs to handle the instruction/mobileInstruction switching which is migrating behaviour that is currently managed in some of the components (narrative/hotgraphic) into the framework as a global feature
  • img loading=eager needs handling in the react templates as it currently is in the handlebars ones
  • I should change the syntax to pass the model, view into the templates rather than view, data

UPDATE: all of the above are implemented in the fw branch issue/2824-jsx

templates/text.jsx Outdated Show resolved Hide resolved
js/adapt-contrib-text.js Show resolved Hide resolved
js/adapt-contrib-text.js Outdated Show resolved Hide resolved
@oliverfoster oliverfoster changed the title part of issue/#2876 issue/2876 React version of text Aug 17, 2020
@ghost ghost requested a review from oliverfoster August 17, 2020 11:12
Copy link

Choose a reason for hiding this comment

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

👀

@oliverfoster oliverfoster added the on hold do not merge label May 10, 2021
@oliverfoster
Copy link
Member

Ready to review, requires pr#3140

Copy link

@ethan-lp ethan-lp left a comment

Choose a reason for hiding this comment

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

👀

js/adapt-contrib-text.js Outdated Show resolved Hide resolved
templates/text.jsx Outdated Show resolved Hide resolved
@oliverfoster oliverfoster changed the title issue/2876 React version of text issue/2876 JSX version of text Aug 26, 2021
@oliverfoster oliverfoster removed the on hold do not merge label Aug 26, 2021
@oliverfoster oliverfoster self-assigned this Aug 26, 2021
Copy link

Choose a reason for hiding this comment

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

👀

Copy link
Contributor

@tomgreenfield tomgreenfield left a comment

Choose a reason for hiding this comment

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

👁️

@oliverfoster oliverfoster merged commit df4f5fb into master Aug 26, 2021
@oliverfoster oliverfoster deleted the issue/2876 branch August 26, 2021 12:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants