-
Notifications
You must be signed in to change notification settings - Fork 92
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
fix: Height in Advanced Editor [FC-0076] #1649
base: master
Are you sure you want to change the base?
fix: Height in Advanced Editor [FC-0076] #1649
Conversation
Thanks for the pull request, @ChrisChV! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1649 +/- ##
=======================================
Coverage 93.24% 93.24%
=======================================
Files 1100 1101 +1
Lines 21803 21860 +57
Branches 4632 4644 +12
=======================================
+ Hits 20330 20384 +54
- Misses 1408 1411 +3
Partials 65 65 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 to making the editor modal larger -- that makes it easier to edit the most commonly-used XBlocks, though it's a bit weird for the ones tested here, as they don't have enough fields to fill the full 80vh.
e.g. Google Document:
PDF:
Completion:
- I tested this using the PR test instructions.
- I read through the code
-
I checked for accessibility issuesN/A -
Includes documentationN/A -
User-facing strings are extracted for translationN/A
@@ -28,7 +28,9 @@ export const LibraryBlock = ({ | |||
view, | |||
}: LibraryBlockProps) => { | |||
const iframeRef = useRef<HTMLIFrameElement>(null); | |||
const [iFrameHeight, setIFrameHeight] = useState(50); | |||
const defaultiFrameHeight = view === 'studio_view' ? 80 : 50; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think this would be clearer if you moved the line from below up here and used its value:
const defaultiFrameHeight = view === 'studio_view' ? 80 : 50; | |
const xblockView = view ?? 'student_view'; | |
const defaultiFrameHeight = xblockView === 'studio_view' ? 80 : 50; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed bfc12d1
Description
Supporting information
Testing instructions
Other information
N/A