Fix #5982: Fixes modal overflow issue by using word-break property #5983
Conversation
|
@bansalnitish There are a few commits I have to make right now. Kindly review only after I confirm finishing the solution. |
|
Hey @rakshitkumarcse, do you mind changing the PR title to start with WIP i.e work in progress. (Remove -- when its ready for review)? |
|
Sure @bansalnitish |
mighty-phoenix
changed the title
Codecov Report
@@ Coverage Diff @@
## develop #5983 +/- ##
=======================================
Coverage 45.3% 45.3%
=======================================
Files 520 520
Lines 30667 30667
Branches 4597 4597
=======================================
Hits 13893 13893
Misses 16774 16774Continue to review full report at Codecov.
|
|
Alright. While fixing this issue, I came through a few more major front-end issues:
After fixing, it appears like this now.
Now, after fixing, it appears like this:
Now as for the main issueEarlier, in multiple choice interaction, "add response" modal looked like this:
Now, after fixing, it appears like this:
What are your views on it? @seanlip @bansalnitish |
mighty-phoenix
changed the title
There was a problem hiding this comment.
Hi @rakshitkumarcse,
Left some comments for you in my quick pass over the code. I'll need some time to test this over multiple cases and across devices (mobile and desktop) but firstly we'll need to decide over my last comment.
Also, your last screenshot is not convincing. The dropdown menu is just flowing out of the Add Response modal, its looking somewhat bad.
Thanks
| </div> | ||
| <span class="caret oppia-html-select-selection-caret"></span> | ||
| </button> | ||
| <ul uib-dropdown-menu style="width: inherit;"> | ||
| <ul uib-dropdown-menu style="max-width: 1000px; width: auto; overflow: auto;"> |
There was a problem hiding this comment.
Why the max-width: 1000px ?
There was a problem hiding this comment.
@bansalnitish I see your view upon this. Should I change the drop-down box length to a length such that it stays uniformly inside the modal?
There was a problem hiding this comment.
@bansalnitish I made a change. It looks pretty fine now.
Have a look.
Thanks.
| @@ -1,7 +1,7 @@ | |||
| <!-- label-for-focus-target feature is removed since CKEditor has a | |||
| focusManager to manage the focus activity automatically. --> | |||
| <ck-editor-rte ng-if="!isDisabled()" ng-model="$parent.localValue" | |||
| ui-config="uiConfig()"> | |||
| ui-config="uiConfig()" style="word-break: break-all;"> | |||
There was a problem hiding this comment.
Do you really need word-break here as well ? (I'm not sure but I myself just don't want to add here)
There was a problem hiding this comment.
@bansalnitish If we don't use word-break here, we will need to change it specifically for different interactions, which didn't seem correct to me because in the coming time, if we plan to add more kinds of interactions, we will have to specify word-break everywhere then. Moreover, in editor, I can't think of a case when we really don't want the editor to not do word-break and increase it's own length.
Thanks!
There was a problem hiding this comment.
@bansalnitish And not only interactions, but we will also have to specify word break for all places where editor is used explicitly if we don't specify it here.
The introduction modal looked like this before.
Now it looks like this
Thanks!
There was a problem hiding this comment.
Is it possible to add this as a global style for the ck-editor-rte tag? Or do we not want this for all instances of the RTE?
There was a problem hiding this comment.
I think there is no such instance of rte where this style is not to be followed.
Kindly tell me your views on it so that I can make it global. Currently, I just saw that there are other places where the same error is occuring. Such as feedback tab. So should I undo the changes I did in this PR regarding this paticular problem and make a new issue for it? Because this issue seems pretty vast.
There was a problem hiding this comment.
Alright. Will make the changes in css itself. Thanks! @nithusha21
There was a problem hiding this comment.
@nithusha21 The problem is that there isn't only 1 class for editors and editor content holders. There are a number of classes for that. So making changes in css will take some time. I'd rather suggest you to merge this PR, and then we can work on it making a seperate issue. What do you think about it?
There was a problem hiding this comment.
@nithusha21 Would you like me to undo the changes to editor and editor content holders in this PR, make a new issue, and work on all the editor classes and their content holders using oppia.css?
There was a problem hiding this comment.
Hmmm. I see. I was under the impression that it was a single class throughout. Also I just went through the code involved, and it looks like the style tag was used on many different components as you said.
Just to confirm one thing, All the editors would be ck-editor-rte element itself right?
I agree the display for the content will be different, and I think the cleanest way to do this global style would be to use a common class for all of these tags, and add common styles in oppia.css.
If we actually defer this to another PR, I think that PR would revert a lot of the style changes here, and add it elsewhere. So I think it would make sense to do it here itself? (As anyway these changes would be removed). What do you think? However, I think if we do the global approach, we would need to test it carefully, and make sure all RTEs continue to work properly.
I want to commend you on doing a great job of identifying all the RTEs and carefully testing their behavior. Well done!
There was a problem hiding this comment.
@nithusha21 Thanks! I don't have any problem in working on the changes in this issue itself. It might take a few days to carefully make the changes and test them.
| @@ -1,12 +1,11 @@ | |||
| <div uib-dropdown class="oppia-html-select"> | |||
| <button uib-dropdown-toggle class="btn btn-default oppia-html-select-selection" type="button" aria-expanded="false" data-toggle="dropdown"> | |||
| <div> | |||
| <angular-html-bind class="oppia-html-select-selection-element" html-data="options[getSelectionIndex()].val"> | |||
| </angular-html-bind> | |||
| Click here to select | |||
There was a problem hiding this comment.
Why are you changing the existing view? I mean in your case the author would see the Click here to select whereas in original he would see the first option.
I feel, we should keep the original view intact and probably increase the size of the box (where you currently right click here to select) according to the length of first option. The bigger the option the bigger the box gets (using word-break etc.). We can discuss upon this one.
There was a problem hiding this comment.
@bansalnitish We can rather change the existing view to a view that initially shows the first option, but truncated with 3 dots at the end. In as many websites I have seen till date, none of them increase the size of the drop-down select box. It doesn't look clean to me. What do you think?
Thanks!
There was a problem hiding this comment.
@seanlip Would also like to take your suggestion here.
There was a problem hiding this comment.
I think it would be better to have it truncated in the display. Having a horizontally scrollable dropdown looks weird. (Correct me if I'm wrong, but from the screenshots it looks like it is horizontally scrollable right?). Also, I don't think this is very likely to happen in production as it is not normal to have such long words.
Or is it possible to add something similar to word-break to get the same functionality as the rtes? I am happy either way, but yeah, I would prefer the word-break approach so that it looks similar on all views.
There was a problem hiding this comment.
@nithusha21 Alright. Working on the changes.
There was a problem hiding this comment.
@nithusha21 @bansalnitish
Does this look fine now?
There was a problem hiding this comment.
Can we match the size of the dropdown itself, and the place that is used to open the dropbox? Other than that I think it looks ok.
There was a problem hiding this comment.
@nithusha21 size of dropdown is matched now. Have a look. Thanks.
There was a problem hiding this comment.
Looks better now! Thanks :)
|
@bansalnitish I think I am done with all corrections, kindly give the final review(if needed, so that I can make any more changes) and merge the PR. |
mighty-phoenix
changed the title
mighty-phoenix
changed the title
|
@nithusha21 @apb7 @bansalnitish This PR is ready for merge. Kindly do final testing and merge. |
There was a problem hiding this comment.
Hi @rakshitkumarcse thanks for the changes. I would defer to @bansalnitish for final testing/approval. I am traveling this weekend and would not be able to get back to this before Monday. Let me know if you have any specific design questions. If not, and if @bansalnitish is happy, feel free to merge!
|
@bansalnitish Although I have tested all the changes on my own, I'd like you to do the final testing. |
|
@nithusha21 Since @bansalnitish seems inactive currently, I request you to do the final testing and merge. |
|
cc/ @nithusha21 |
|
Hi @rakshitkumarcse really sorry for the delay and confusion! I will test this tomorrow morning. Thanks! |
|
@nithusha21 It's totally fine. Thanks for informing. |
|
@nithusha21 Just a gentle reminder. Kindly test. I think that this is a really important PR for the frontend, good user experience and needs to be put to production. |
There was a problem hiding this comment.
Hi @rakshitkumarcse I think this PR looks great! I tested the following scenarios:
- Long single words in the editor, normal text in the editor
- Item selection input interaction input, answer group rules.
- Multiple choice input interaction input and rules
- hints
Any location I missed?
| @@ -70,7 +77,7 @@ | |||
| ng-click="navigateToState(getOutcome().dest)" class="oppia-nested-link"> | |||
| <[getOutcome().dest]> | |||
| </span> | |||
| <span ng-if="getOutcome() && !isOutcomeLooping() && isCreatingNewState()"> | |||
| <span class="oppia-nested-link" ng-if="getOutcome() && !isOutcomeLooping() && isCreatingNewState()"> | |||
There was a problem hiding this comment.
@nithusha21
It fixed this behaviour. Earlier, the text that denoted new card name overlapped with the close icon. Now, introduction of the oppia-nested-link class is exactly what we need. It uses ellipsis for text overflow. So, I prevented making a new class unnecesarily.
Before:
After:
There was a problem hiding this comment.
Thanks for the confirmation. This sounds good! Good catch btw :)
|
Sorry for the delay! Something came up at home, was busy! Just got back and logged in :) |
|
@nithusha21 Thanks for the review. |
|
@nithusha21 Also, the schema_based_html_editor_directive file shows up in files changed with no change because I had changed the file in an earlier commit, but had undone the change in a later commit, to do the styling in oppia.css instead of schema_based_html_editor_directive file. |
|
@nithusha21 Sorry for the spelling mistake above. I meant translation tab and not statistics tab. |
There was a problem hiding this comment.
I think this looks great! Thanks @rakshitkumarcse. Also @bansalnitish do you want to take a final look here?
|
@nithusha21 Thanks for the review. |
|
Please note that this PR is currently breaking explorations in production on Firefox.
@rakshitkumarcse @bansalnitish I think we need to fix this ASAP. Also, QA team -- please note that this break is serious and requires a hotfix. Could we ensure that Firefox is included in our testing process? /cc @nithusha21 |
|
@seanlip Would hyphenation at word break be a correct behavior? Or are we expecting shifting of the whole word to the next line? |
|
Not in this case -- "nu-mbers" would still be weird. I would expect the whole word to shift to the next line. (Wouldn't you?) |
|
@seanlip Even I think we should shift the whole word down. I will make a PR for the same. |
Fixes #5982
Explanation
Used word-break property with value "break-all" to avoid modal overflow.
Checklist
python scripts/pre_commit_linter.pyandbash scripts/run_frontend_tests.sh.