prevent Dialog from being uncloseable with long text #8475

cezaraugusto · 2017-04-24T19:58:56Z

Submitted a ticket for my issue if one did not already exist.
Used Github auto-closing keywords in the commit message.
Added/updated tests for this change (for new code or code which already has tests).
Ran git rebase -i to squash commits (if needed).
Fix #7930
Auditors: @luixxiul

Screenshot:

Note: no scrollbar is shown because I have it hidden by default in macOS. It's set as default so each OS will render native scrollbars (per Brad #8475 (review))

/cc @bradleyrichter for design review

Test Plan:

Open https://jsfiddle.net/c5o7arqu/
Make the browser window small
Click "OK"
Scrollbar should show, text should be scrollable
Remove some text with devTools or open a smaller dialog
Scrollbar shouldn't show
everything should be properly aligned
Resizing screen should resize dialog as well (responsive)

jonathansampson · 2017-04-24T20:12:10Z

@cezaraugusto @bradleyrichter Can we consider making only the body text scrollable (when it hits a max-height), so the dialog button is always visible? Making the user scroll down to dismiss the dialog seems unnecessary.

NejcZdovc · 2017-04-24T20:26:09Z

Personally I wouldn't go with a full width (90%), but limit it to I don't know maybe max 300, 400px (completely random number from the top of my head 😃 )

cezaraugusto · 2017-04-24T20:26:56Z

@jonathansampson that's a great idea. I'll wait for design feedback from Brad and do all together, thanks for reviewing

cezaraugusto · 2017-04-24T20:28:18Z

@NejcZdovc thanks, yep I did a random guess as well

jonathansampson · 2017-04-24T20:35:28Z

Agree with @NejcZdovc that we need a max-width, or a fixed-width.

luixxiul · 2017-04-25T07:56:46Z

+1 unless the title could be so long that it would overflow the dialog.

luixxiul · 2017-04-25T10:51:20Z

app/renderer/components/styles/commonStyles.js

+    '::-webkit-scrollbar-thumb': {
+      background: globalStyles.color.braveOrange,
+      // same as primary button
+      boxShadow: '0px 1px 5px -1px rgba(0, 0, 0, 0.5)',


let's replace the box-shadow with a variable on global.js for future use :-)

luixxiul · 2017-04-25T10:51:30Z

app/renderer/components/styles/commonStyles.js

+      borderRadius: '4px'
+    },
+    '::-webkit-scrollbar-track': {
+      boxShadow: 'inset 0 0 4px rgba(0,0,0, 0.3)',


bradleyrichter · 2017-04-25T11:30:32Z

+++

…

On Apr 25, 2017, at 3:51 AM, Suguru Hirahara ***@***.***> wrote: @luixxiul commented on this pull request. In app/renderer/components/styles/commonStyles.js: > + top: globalStyles.spacing.dialogTopOffset, + // Issue #7930 + overflowY: 'auto', + overflowX: 'hidden', + maxWidth: '90%', + maxHeight: '90%', + boxSizing: 'border-box', + + // Add a custom scrollbar for the element + '::-webkit-scrollbar': { + width: '10px' + }, + '::-webkit-scrollbar-thumb': { + background: globalStyles.color.braveOrange, + // same as primary button + boxShadow: '0px 1px 5px -1px rgba(0, 0, 0, 0.5)', let's replace the box-shadow with a variable on global.js for future use :-) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

cezaraugusto · 2017-04-26T09:44:37Z

Updated, set a smaller size and migrated scroll to body

luixxiul · 2017-04-26T10:57:25Z

Isn't the scroll bar too low contrasted?

cezaraugusto · 2017-04-26T11:32:40Z

contrast looks good for me but I don't mind changing it

jonathansampson · 2017-04-27T01:54:55Z

@cezaraugusto Looks really great! Can we do anything about making sure it isn't taller than the window?

jonathansampson

Everything looks good, and works well. I had one suggestion to further improve, but I still think this is ship-ready.

luixxiul · 2017-04-27T02:36:24Z

app/renderer/components/common/messageBox.js

@@ -154,7 +158,10 @@ const styles = StyleSheet.create({
    marginTop: globalStyles.spacing.dialogInsideMargin,
    minWidth: '425px',
    marginBottom: globalStyles.spacing.dialogInsideMargin,
-    userSelect: 'text'
+    userSelect: 'text',
+    maxHeight: '300px',


what if we would apply vh instead of px? 300px might be small for full HD monitors. Please see about:error page for examples.

Oh, good idea @luixxiul with vh. Since dialogs are pretty much always sized and positioned relatively to the viewport, it makes sense to use vh and vw. What do you think, @cezaraugusto?

that's great, I took a look at other browsers and they take that approach as well. Maybe 80vh is good enough?

luixxiul · 2017-04-27T02:36:32Z

app/renderer/components/styles/commonStyles.js

+    // Issue #7930
+    boxSizing: 'border-box',
+    maxWidth: '600px',
+    maxHeight: '500px'


bradleyrichter

After some thought, I think it would be desirable to use the native scrollbar on each platform so that users recognize it as being a standard control used by the OS.


        prevent Dialog from being uncloseable with long text

@luixxiul

- Fix #7930 - Auditors: @luixxiul


        reopen

cezaraugusto · 2017-04-28T05:16:15Z

PR updated with custom scrollbar removal. New STR and preview here: #8475 (comment)

cezaraugusto · 2017-04-28T05:22:51Z

app/renderer/components/styles/global.js

@@ -110,7 +110,7 @@ const globalStyles = {
    buttonHeight: '25px',
    buttonWidth: '25px',
    navbarHeight: '36px',
-    downloadsBarHeight: '50px',


looking at the component and at variables.less it actually never was 50px

jonathansampson · 2017-04-28T19:05:23Z

Just pulled down the changes; beautiful work, @cezaraugusto!

bsclifton

Awesome work, @cezaraugusto! This looks and feels great 😄

cezaraugusto added accessibility bug labels Apr 24, 2017

cezaraugusto added this to the 0.15.1 milestone Apr 24, 2017

cezaraugusto requested review from luixxiul and bradleyrichter Apr 24, 2017

luixxiul reviewed Apr 25, 2017

View changes

app/renderer/components/styles/commonStyles.js Outdated

borderRadius: '4px'

},

'::-webkit-scrollbar-track': {

boxShadow: 'inset 0 0 4px rgba(0,0,0, 0.3)',

This comment has been minimized.

Sign in to view

Copy link

luixxiul Apr 25, 2017
Contributor

ditto.

cezaraugusto self-assigned this Apr 26, 2017

cezaraugusto mentioned this pull request Apr 26, 2017

Create ScrollableContent element #8505

Closed

cezaraugusto force-pushed the common/dialog/7930 branch 2 times, most recently from 771c65c to 633987e Apr 26, 2017

cezaraugusto added the PR/needs-QA-attention ☕ label Apr 26, 2017

cezaraugusto requested review from jonathansampson, NejcZdovc and bsclifton Apr 26, 2017

jonathansampson approved these changes Apr 27, 2017

View changes

luixxiul reviewed Apr 27, 2017

View changes

app/renderer/components/styles/commonStyles.js Outdated

// Issue #7930

boxSizing: 'border-box',

maxWidth: '600px',

maxHeight: '500px'

This comment has been minimized.

Sign in to view

Copy link

luixxiul Apr 27, 2017
Contributor

ditto

cezaraugusto mentioned this pull request Apr 27, 2017

Split scroll between titles and folders for BM manager #8532

Merged

3 of 4 tasks complete

bradleyrichter reviewed Apr 27, 2017

View changes

cezaraugusto closed this Apr 28, 2017

cezaraugusto force-pushed the common/dialog/7930 branch from 633987e to 8da5411 Apr 28, 2017

cezaraugusto added 2 commits Apr 28, 2017

prevent Dialog from being uncloseable with long text

Verified

This commit was signed with a verified signature.

cezaraugusto Cezar Augusto

GPG key ID: ECC5FF1C7EA1D39D Learn about signing commits

c4b43cf

- Fix #7930 - Auditors: @luixxiul

reopen

Verified

This commit was signed with a verified signature.

cezaraugusto Cezar Augusto

GPG key ID: ECC5FF1C7EA1D39D Learn about signing commits

Loading status checks…

592db4b

cezaraugusto reopened this Apr 28, 2017

cezaraugusto reviewed Apr 28, 2017

View changes

cezaraugusto merged commit 1ebea19 into master Apr 28, 2017
0 of 2 checks passed

0 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details

continuous-integration/travis-ci/pr The Travis CI build failed
Details

cezaraugusto deleted the common/dialog/7930 branch Apr 28, 2017

luixxiul mentioned this pull request Apr 29, 2017

Alert box with quite lengthy content cannot be closed #7930

Closed

bsclifton reviewed Apr 30, 2017

View changes

luixxiul removed the PR/needs-QA-attention ☕ label Apr 30, 2017

luixxiul removed the request for review from NejcZdovc Sep 7, 2017

brave / browser-laptop Archived

prevent Dialog from being uncloseable with long text #8475

prevent Dialog from being uncloseable with long text #8475

cezaraugusto commented Apr 24, 2017 •

edited by luixxiul

jonathansampson commented Apr 24, 2017 •

edited

NejcZdovc commented Apr 24, 2017 •

edited

cezaraugusto commented Apr 24, 2017

cezaraugusto commented Apr 24, 2017

jonathansampson commented Apr 24, 2017

luixxiul commented Apr 25, 2017 •

edited

This comment has been minimized.

This comment has been minimized.

bradleyrichter commented Apr 25, 2017

cezaraugusto commented Apr 26, 2017

luixxiul commented Apr 26, 2017

cezaraugusto commented Apr 26, 2017

jonathansampson commented Apr 27, 2017 •

edited

jonathansampson left a comment •

edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bradleyrichter left a comment

cezaraugusto commented Apr 28, 2017

This comment has been minimized.

jonathansampson commented Apr 28, 2017

bsclifton left a comment

brave / browser-laptop Archived

Join GitHub today

prevent Dialog from being uncloseable with long text #8475

prevent Dialog from being uncloseable with long text #8475

Conversation

cezaraugusto commented Apr 24, 2017 • edited by luixxiul

jonathansampson commented Apr 24, 2017 • edited

NejcZdovc commented Apr 24, 2017 • edited

cezaraugusto commented Apr 24, 2017

cezaraugusto commented Apr 24, 2017

jonathansampson commented Apr 24, 2017

luixxiul commented Apr 25, 2017 • edited

This comment has been minimized.

luixxiul Apr 25, 2017

This comment has been minimized.

luixxiul Apr 25, 2017

bradleyrichter commented Apr 25, 2017

cezaraugusto commented Apr 26, 2017

luixxiul commented Apr 26, 2017

cezaraugusto commented Apr 26, 2017

jonathansampson commented Apr 27, 2017 • edited

jonathansampson left a comment • edited

This comment has been minimized.

luixxiul Apr 27, 2017 • edited

This comment has been minimized.

jonathansampson Apr 27, 2017

This comment has been minimized.

cezaraugusto Apr 27, 2017

This comment has been minimized.

luixxiul Apr 27, 2017

bradleyrichter left a comment

cezaraugusto commented Apr 28, 2017

This comment has been minimized.

cezaraugusto Apr 28, 2017

jonathansampson commented Apr 28, 2017

bsclifton left a comment

cezaraugusto commented Apr 24, 2017 •

edited by luixxiul

jonathansampson commented Apr 24, 2017 •

edited

NejcZdovc commented Apr 24, 2017 •

edited

luixxiul commented Apr 25, 2017 •

edited

jonathansampson commented Apr 27, 2017 •

edited

jonathansampson left a comment •

edited

luixxiul Apr 27, 2017 •

edited