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

Address 4091/RCT #18: Set max-height for ItemSelectionInputResponse images #4229

Merged
merged 2 commits into from
Dec 18, 2017
Merged

Address 4091/RCT #18: Set max-height for ItemSelectionInputResponse images #4229

merged 2 commits into from
Dec 18, 2017

Conversation

AllanYangZhou
Copy link
Contributor

Before
before

After
after

seanlip
seanlip reviewed Dec 18, 2017
View reviewed changes
...ions/interactions/ItemSelectionInput/directives/item_selection_input_response_directive.html Outdated

<style>
.item-selection-input-response-content img {
max-height: 150px;
Copy link
Member

Choose a reason for hiding this comment

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

I think maybe this could be a bit smaller?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

150->100
150_2_100

@AllanYangZhou
Copy link
Contributor Author

@seanlip Also added a limit of 200px for all itemselectioninput images (see OP for example images)

seanlip
seanlip reviewed Dec 18, 2017
View reviewed changes
extensions/interactions/ItemSelectionInput/static/item_selection_input.css
@@ -36,6 +36,10 @@ md-checkbox.item-selection-input-checkbox.md-default-theme.md-checked .md-icon {
border-bottom: 0px;
}

.item-selection-input-container img {
max-height: 200px;
Copy link
Member

@seanlip seanlip Dec 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

For this part, don't you need to set max-width as well (so it doesn't spill out of the card)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, in the images I've tried it doesn't overflow the width. At least in FF, that seems to be enforced by this.

It looks like the card itself can get cut off if the screen is too small, but that's a separate issue.

seanlip
seanlip approved these changes Dec 18, 2017
View reviewed changes
Copy link
Member

@seanlip seanlip left a comment

Choose a reason for hiding this comment

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

LGTM, just one more small thing. Thanks!

@seanlip
Copy link
Member

seanlip commented Dec 18, 2017

Ah ok, sounds good then. Thanks @AllanYangZhou!

@seanlip seanlip mentioned this pull request Dec 18, 2017
Closed
4 tasks
@codecov-io
Copy link

codecov-io commented Dec 18, 2017 •
edited
Loading

Codecov Report

Merging #4229 into develop will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #4229   +/-   ##
========================================
  Coverage    44.51%   44.51%           
========================================
  Files          367      367           
  Lines        22517    22517           
  Branches      3578     3578           
========================================
  Hits         10023    10023           
  Misses       12494    12494

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7500f0a...10cc93e. Read the comment docs.

@seanlip seanlip merged commit 371570b into oppia:develop Dec 18, 2017
@seanlip seanlip mentioned this pull request Dec 18, 2017
Closed
giritheja added a commit to giritheja/oppia that referenced this pull request Dec 22, 2017
@giritheja
Merge remote-tracking branch 'upstream/develop' into questions-projec…
1b5106c 
…t-p2

* upstream/develop: (124 commits)
  Introduce a correctness_feedback_enabled field at the exploration level and make corresponding updates to the states dict. (oppia#4235)
  Migrate next_skill_id to next_skill_index. (oppia#4237)
  Fix oppia#4224: Set Admin Badge in place. (oppia#4236)
  Fix part of oppia#3950: Removed jinja templates in settings_tab in exploration editor (oppia#4209)
  Routine update of translations. (oppia#4234)
  Refactor outcomes, hints, and solutions to use SubtitledHtml (oppia#4233)
  Fix oppia#4202: Show Hints and Solutions in modals (oppia#4203)
  Fixes 3741: Remove action parameter when moderator unpublishes exploration. (oppia#4213)
  Address 4091/RCT oppia#18: Set max-height for ItemSelectionInputResponse images (oppia#4229)
  Add a job to generate statistics models for explorations which do not have them yet. (oppia#4228)
  Fix a bunch of issues related to the maintenance.html page not loading Angular properly. (oppia#4227)
  Fix some errors in e2e tests discovered during release testing. (oppia#4226)
  Update AUTHORS, CONTRIBUTORS, about.html, and CHANGELOG for the 2.5.7 release (oppia#4225)
  Simplify patterns for using promises in the codebase. (oppia#4204)
  fix for oppia#4144 - misaligned data (oppia#4205)
  Fix oppia#4216 and oppia#4218: link correctly to static assets in interactions. (oppia#4219)
  Fix one last error in the GenerateStatsV1Job (oppia#4215)
  Fix oppia#3891: pattern check for directly referenced directives implemented. (oppia#4187)
  convert str versions to int (oppia#4210)
  Fix error in GenerateV1StatisticsJob (oppia#4208)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seanlip seanlip seanlip approved these changes

@tjiang11 tjiang11 Awaiting requested review from tjiang11

Assignees
No one assigned
Labels
PR: LGTM
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@AllanYangZhou @seanlip @codecov-io