After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 785879 - Behaviour when fullscreen and resizing could be improved
Behaviour when fullscreen and resizing could be improved
Status: RESOLVED FIXED
Product: gnome-characters
Classification: Other
Component: general
3.24.x
Other Linux
: Normal normal
: ---
Assigned To: GNOME Characters maintainer(s)
GNOME Characters maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2017-08-06 11:54 UTC by Sam P.
Modified: 2017-08-11 09:17 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch for the first issue (3.12 KB, patch)
2017-08-06 11:54 UTC, Sam P.
reviewed Details | Review
Patch for the 2nd issue (1.93 KB, patch)
2017-08-06 11:54 UTC, Sam P.
committed Details | Review
Patch for the first issue; but with "let" instead of "const" (3.11 KB, patch)
2017-08-07 21:26 UTC, Sam P.
committed Details | Review

Description Sam P. 2017-08-06 11:54:26 UTC
Created attachment 357052 [details] [review]
Patch for the first issue

Love gnome characters, just it always bugged me how it behaves poorly on large screens.  The issue is:

1. Open Gnome Characters
2. Maximize the window (on large screen, like 1080p)
3. Click to view a long category ("Pictures")
4. Notice that screen is basically empty, and you can not scroll to load more

The issue also happens when resizing:

1. Open Gnome Characters, leave it at the default size
3. Click to view a long category ("Pictures").  Do not scroll
2. Maximize the window (on large screen, like 1080p)
4. Notice that screen is (still) basically empty, and you can not scroll to load more

Please see the 2 patches provided.
Comment 1 Sam P. 2017-08-06 11:54:56 UTC
Created attachment 357053 [details] [review]
Patch for the 2nd issue
Comment 2 Daiki Ueno 2017-08-07 14:30:15 UTC
Review of attachment 357052 [details] [review]:

Thank you, good point.  The patch looks fine, except a nit:

::: src/characterList.js
@@ +507,3 @@
+        // Use our parents allocation; we aren't visible before we do the
+        // initial search, so our allocation is 1x1
+        const allocation = this.get_parent().get_allocation();

"let" seems more appropriate here (and the 3 occurrences below) than "const", because the values are not constant.
Comment 3 Daiki Ueno 2017-08-07 14:30:57 UTC
Review of attachment 357053 [details] [review]:

Looks good to me.
Comment 4 Sam P. 2017-08-07 21:26:06 UTC
Thanks, I've attached a version of the first patch that uses "let" instead of "const".

Note that "const" is not just for constants.  The AirBnB style guide for example [1] says that it should be used as the default.  It just stops you from reassigning to the variable; making the flow of the program much clearer.

[1]  https://github.com/airbnb/javascript#references
Comment 5 Sam P. 2017-08-07 21:26:38 UTC
Created attachment 357157 [details] [review]
Patch for the first issue; but with "let" instead of "const"
Comment 6 Daiki Ueno 2017-08-11 09:17:15 UTC
Thank you, pushed the patches.