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 optional figure reference baking injected exercises #432

Merged
merged 19 commits into from
Nov 12, 2021

Conversation

ajluk
Copy link
Contributor

@ajluk ajluk commented Oct 27, 2021

@ajluk ajluk self-assigned this Oct 27, 2021
@codecov
Copy link

codecov bot commented Oct 27, 2021

Codecov Report

Merging #432 (6fda1d7) into main (3f72d38) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #432   +/-   ##
=======================================
  Coverage   99.96%   99.96%           
=======================================
  Files         179      181    +2     
  Lines        2712     2739   +27     
=======================================
+ Hits         2711     2738   +27     
  Misses          1        1           
Impacted Files Coverage Δ
lib/kitchen/element_base.rb 100.00% <ø> (ø)
lib/kitchen/directions/bake_injected_exercise.rb 100.00% <100.00%> (ø)
...jected_exercise/bake_injected_exercise_question.rb 100.00% <100.00%> (ø)
lib/kitchen/element_enumerator_base.rb 100.00% <100.00%> (ø)
lib/kitchen/injected_exercise_element.rb 100.00% <100.00%> (ø)
...ib/kitchen/injected_exercise_element_enumerator.rb 100.00% <100.00%> (ø)
lib/kitchen/injected_question_element.rb 100.00% <100.00%> (ø)
lib/kitchen/selectors/base.rb 100.00% <100.00%> (ø)
lib/kitchen/selectors/standard_1.rb 100.00% <100.00%> (ø)

Copy link
Contributor

@kswanson33 kswanson33 left a comment

Choose a reason for hiding this comment

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

I think a better place for this change would be in the BakeInjectedExercises class -- this is where the 'Refer to...' text is baked in the first place! I assume this change would only be for single-part exercises?

#
# @return [Element]
#
def exercise_context
Copy link
Contributor

Choose a reason for hiding this comment

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

this should probably go on the InjectedExerciseElement class -- if it doesn't exist, it seems like it should be created

@@ -41,11 +42,15 @@ def bake(question:, number:, only_number_solution:)
end
end

context = question.ancestor(:injected_exercise).exercise_context&.cut&.paste if figure_reference
Copy link
Contributor Author

@ajluk ajluk Nov 2, 2021

Choose a reason for hiding this comment

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

@kswanson33 I have tried refactored the code with your requested changes but I've got stuck in this particular place and got such error:
image
In my recipe I'm using this loop, where it is not pointing to injected_exercises but exercise_question``:
image
and I guess this is the reason why I can't select an ancestor for the exercise_question of given type (`:injected_exercise`). Do you think there is a need to change the `BakeInjectedExerciseQuestion` to point `injected_exercises` instead of `exercise_question` I'm asking because it will need to be updated in some of the recipes? I can also use `context = question.parent.first("div[data-type='exercise-context']")&.cut&.paste if figure_reference` instead (it's working properly then). Or maybe you can see a different approach here, rewrite the loop in the recipe somehow? Let me know if you will have some suggestions. 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kswanson33 could you also explain what did you mean by single-part exercises?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Small EDIT:
I have also tried to use the element class InjectedExerciseElement itsef:
image
but also got error:
image

@ajluk ajluk requested a review from kswanson33 November 2, 2021 19:57
@ajluk ajluk marked this pull request as draft November 5, 2021 18:39
@ajluk ajluk marked this pull request as ready for review November 12, 2021 18:51
@ajluk
Copy link
Contributor Author

ajluk commented Nov 12, 2021

Hi @kswanson33 After Larissa's comment I have reworked the PR to only move the exercise-context if there is only one exercise-question per injected-exercise so this feature will be for now available only for singular part exercises. I have also confirmed it with Larissa. I was also thinking if there will be more exercises questions when there is a exercise-context figure present in some books we perhaps should tag the exercise question with some class, so we will know where to move the exercise-context figure reference but Larissa needs to look into it deeper how the content will look.

Copy link
Contributor

@kswanson33 kswanson33 left a comment

Choose a reason for hiding this comment

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

looks really good! two little comments but other than that it's fantastic :)

CHANGELOG.md Outdated
@@ -8,7 +8,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

* Add `BakeScreenreaderSpans` direction (minor)
* Fix `BakeIndex` to group terms by character in polish books and transliterate it for others (minor)

* Add `figure_reference` option for `BakeNoteInjectedQuestion` to bake exercise-context figure reference (minor)
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't forget to update the changelog to reflect the new change! I believe this change is major as single-part injected exercises should always be changed.

end

class V1
def bake(question:, number:, only_number_solution:)
def bake(question:, number:, only_number_solution:, figure_reference:)
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need an argument here? probably this should always be true

@ajluk ajluk merged commit 7fddda2 into main Nov 12, 2021
@ajluk ajluk deleted the figure-reference-injected-exercises branch November 12, 2021 19:47
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