Skip to content

Bugfixes - #3

Merged
martynjames merged 3 commits into
masterfrom
mjevtic/workbranch
Dec 4, 2014
Merged

Bugfixes#3
martynjames merged 3 commits into
masterfrom
mjevtic/workbranch

Conversation

@marjev

@marjev marjev commented Nov 24, 2014

Copy link
Copy Markdown
Contributor
  1. Fixed input field for the calendar ID

  2. Adjusted width and height of the images so that they don't overflow outside the XBlock div

  3. Enabled reseting the value to default in studio modal

@martynjames

Copy link
Copy Markdown

Reset seems to work well 😃

A couple of display issues:
i) WordProcessing Document - rendering is constrained too narrowly:
screen shot 2014-12-01 at 3 11 51 pm

ii) Spreadsheet document - rendering is constrained too narrowly:
screen shot 2014-12-01 at 3 16 07 pm

iii) Slides display too small in LMS:
screen shot 2014-12-01 at 3 21 14 pm

iv) Document too narrow in LMS:
screen shot 2014-12-01 at 3 22 10 pm

v) Spreadsheet too narrow in LMS:
screen shot 2014-12-01 at 3 22 48 pm

vi) Form too short in LMS:
screen shot 2014-12-01 at 3 23 27 pm

@martynjames

Copy link
Copy Markdown

tests folder should be beneath google_drive folder instead of a sibling

@marjev marjev changed the title Newest fixes for Google Drive XBlock Bugfixes Dec 2, 2014
@marjev

marjev commented Dec 2, 2014

Copy link
Copy Markdown
Contributor Author

Squashed all commits to one. Also made changes so that only bugfix commits are included in this pull request.

@martynjames

Copy link
Copy Markdown

Good updates - but issues (iii) and (vi) still exist.

Please address (iii) and (vi) and perform a rebase - the other 2 PRs have been integrated, and there are some merge conflicts

Here a comparison:
screen shot 2014-12-02 at 2 58 25 pm
vs.
screen shot 2014-12-02 at 3 00 04 pm

and

screen shot 2014-12-02 at 2 59 12 pm
vs.
screen shot 2014-12-02 at 3 00 27 pm

Comment thread google_drive/public/js/google_docs.js Outdated

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I feel like this code should be executed only when the document is loaded - I think it should be within the jquery wrapped function below

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Problem is in the way XBlocks load javascript files in the first place. If I understood the way that works, that function will be assigned but not executed on the first load. It will be executed only when the reloading takes place. I struggled with that myself when I was making my very first XBlock. If you have any idea how we could go around that, I'm happy to change it.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I moved it with no problem - I'll update this branch so that you can take a look

@marjev

marjev commented Dec 3, 2014

Copy link
Copy Markdown
Contributor Author

Regarding the slideshows and forms, I tried testing this with the slideshow you first added to the XBlock:

<iframe src="https://docs.google.com/presentation/d/1B22AsMwG7jgE39vm1BuiJClir_JUz1q077cPEKJR2mI/embed?start=false&loop=false&delayms=3000" frameborder="0" width="960" height="569" allowfullscreen="true" mozallowfullscreen="true" webkitallowfullscreen="true"></iframe>

Both slideshows and forms allow users to define the width and height of the embedded iframe explicitly. Perhaps you might try and change that on the Google Drive side and see if the changes take place and if that's a satisfactory solution.

IMO, if user have option to specifically define the width and height on one side, we shouldn't try to affect that via css.

@martynjames

Copy link
Copy Markdown

The problem is the constraint of max-height: 100%; get rid of that, and all seems well. We're constraining BOTH width and height to 100% - if we choose only one (and I'd choose width because of the way both edx UIs have layout) then we won't squish them.

I've added some changes to this branch - @marjev please look and see if you like them - if so, please merge this PR.

@martynjames

Copy link
Copy Markdown

I also squashed some of your commits into one - some of them had interim merge commits with <<<<HEAD etc. markers for conflicts

martynjames pushed a commit that referenced this pull request Dec 4, 2014
@martynjames
martynjames merged commit 737debd into master Dec 4, 2014
@Agrendalath
Agrendalath deleted the mjevtic/workbranch branch February 18, 2025 15:26
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