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

feature: legacy TR-1499 html5+figure/figcaption support #313

Merged

Conversation

pnal
Copy link
Contributor

@pnal pnal commented May 26, 2022

TR-1499
Marshalling/unmarshalling HTML5 figure&figcaption elements

Related PRs:
https://github.com/oat-sa/extension-tao-deliver-connect/pull/125
oat-sa/extension-tao-itemqti#2138

@pnal pnal changed the title feature: legacy TR-1499 html5+figure/figcaption support [Draft] feature: legacy TR-1499 html5+figure/figcaption support May 26, 2022
qtism/data/content/xhtml/html5/Html5LayoutElement.php Outdated Show resolved Hide resolved
qtism/data/content/xhtml/html5/Figcaption.php Outdated Show resolved Hide resolved
qtism/data/content/xhtml/html5/Figure.php Outdated Show resolved Hide resolved
qtism/data/content/xhtml/html5/Figure.php Outdated Show resolved Hide resolved
test/qtismtest/data/content/xhtml/html5/FigcaptionTest.php Outdated Show resolved Hide resolved
test/qtismtest/data/content/xhtml/html5/FigureTest.php Outdated Show resolved Hide resolved
@pnal pnal requested review from kilatib and andreluizmachado June 1, 2022 10:46
@pnal pnal requested a review from kilatib June 1, 2022 12:52
@pnal pnal changed the title [Draft] feature: legacy TR-1499 html5+figure/figcaption support feature: legacy TR-1499 html5+figure/figcaption support Jun 1, 2022
qtism/data/content/xhtml/html5/Figcaption.php Outdated Show resolved Hide resolved
qtism/common/enums/AbstractEnumeration.php Show resolved Hide resolved
qtism/common/enums/AbstractEnumeration.php Show resolved Hide resolved
return static::asArray()[$name] ?? false;
}

public static function getNameByConstant($constant)

Choose a reason for hiding this comment

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

can we add return typehint here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no because we do not have such type string|false

qtism/data/storage/xml/versions/QtiVersion220.php Outdated Show resolved Hide resolved
@pnal pnal requested a review from andreluizmachado June 2, 2022 16:25
@pnal pnal requested a review from andreluizmachado June 8, 2022 08:04
Copy link

@andreluizmachado andreluizmachado left a comment

Choose a reason for hiding this comment

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

  • New code is covered by tests (if applicable)
  • Tests are running successfully (old and new ones) on my local machine (if applicable)
  • New code is respecting code style rules
  • New code is respecting best practices
  • New code is not subject to concurrency issues (if applicable)
  • Feature is working correctly on my local machine (if applicable)
  • Acceptance criteria are respected
  • Pull request title and description are meaningful
  • Changelog is updated according to changes (if applicable)
  • Documentation is updated according to changes (if applicable)

protected $content;

/**
* @param string|null $title A title in the sense of Html title attribute
Copy link
Contributor

Choose a reason for hiding this comment

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

not to require but params could be removed because described in a function head

/**
* @param DOMElement $element
* @param QtiComponentCollection $children
* @return mixed
Copy link
Contributor

Choose a reason for hiding this comment

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

Return value is incorrect

/**
* Checks that the given value is null or one of the enumeration constants.
*
* @param int|string|null $value
Copy link
Contributor

Choose a reason for hiding this comment

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

Not required; params could be removed because described in function head

@pnal pnal merged commit 798edd1 into legacy Jun 10, 2022
@julien-sebire julien-sebire deleted the feature/legacy/TR-1499/support_qti_figure_figcaption_elements branch January 31, 2023 09:39
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