Fix incorrect display of messages on multiple lines #226
Conversation
rugk
changed the title
|
Hi @lazy-sunshine, I'll do a review soon and will later also test out the changes. But the screenshots look great/promising, already, thanks. 😃 Please do also note one important precautionary warning of me: In case you did not notice, you have committed (i.e. created commits) with potentially sensitive data like your (full/private) name or mail. So if this was unintentional and you want that data to be changed/removed first, better do it now. 😃 Here is a good resource (actually from GitHubs help) on how to change your name/mail afterwards. So far just the possibilities. I just wanted to serve this as a friendly reminder, so you don't notice this only when it's too late. What you'll do depends solely on you. 😄 |
| @@ -223,6 +240,8 @@ a:active { | |||
| margin: 4px; | |||
| } | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
Please remove these empty lines you've added. 😄
| @@ -171,6 +173,21 @@ a:active { | |||
| margin-top: 8px; | |||
| } | |||
|
|
|||
| @media screen and (max-width: 767px) { | |||
There was a problem hiding this comment.
Hmm, a fixed break point… Is this really good?
I mean, it should automatically wrap depending on the browser size, so we likely need something (more) responsive there, because:
- in other languages, there may be more text, so even for >767px the text may wrap
- for other screen sizes (widths) and other text contents this whole point, where it needs to break may change, too
Generally, consider that the same code here is embedded for all places where such messages could be appear. Also, I likely will adapt/move it to all other add-on's too, as they use the same code. (see TinyWebEx/common#6)
So this should work at all screen sizes and with any content, if possible…
There was a problem hiding this comment.
@rugk I dnt think its possible that I can change element css based on its sibling element having multiline text,
but I can do a mobile first approach i.e with a min-width media query and if the button does nt fit in single line due to small size then I will move it to next line else for all other cases it could be singleline css only.
please suggest otherwise
There was a problem hiding this comment.
Hmm I also dunno the best way. You could try to have a look on how Firefox implements it on addons.mozilla.org e.g. (look for an unsupported old extension e.g., that should have a red error message).
Or in Firefox itself (about:debugging or about:addons may be good canidates if you e.g. trigger an error).
Or in the source code… I've did search it in a similar area before, so maybe near here you can find something.
If you don't find a better way, yeah, maybe the mobile-first idea is the best. (if I understood your idea correctly)
|
Uhm sorry for the late review, forgot to click to submit the review. |
|
@lazy-sunshine Are you still interested in completing this? If not, that's no problem, just add a notice, so I know what the current status of this PR is. |
| @@ -171,6 +173,21 @@ a:active { | |||
| margin-top: 8px; | |||
| } | |||
|
|
|||
| @media screen and (max-width: 767px) { | |||
There was a problem hiding this comment.
Hmm I also dunno the best way. You could try to have a look on how Firefox implements it on addons.mozilla.org e.g. (look for an unsupported old extension e.g., that should have a red error message).
Or in Firefox itself (about:debugging or about:addons may be good canidates if you e.g. trigger an error).
Or in the source code… I've did search it in a similar area before, so maybe near here you can find something.
If you don't find a better way, yeah, maybe the mobile-first idea is the best. (if I understood your idea correctly)
There was a problem hiding this comment.
@lazy-sunshine If you need any pointers/help or have questions, feel free to ask.
| @@ -223,6 +240,8 @@ a:active { | |||
| margin: 4px; | |||
| } | |||
|
|
|||
|
|
|||
|
@lazy-sunshine Are you still interested in completing this? If not, that's no problem, just add a notice, so I know what the current status of this PR is. |
Fixes #12
UI changes done for same.
@rugk attaching screenshots for Ui display on diff screen width.Please suggest any changes if needed