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

[stable13] Fix breadcrumbs width calculation #8610

Merged
merged 13 commits into from Mar 5, 2018

Conversation

Projects
None yet
3 participants
@danxuliu
Member

danxuliu commented Mar 1, 2018

Backport of #8588

danxuliu added some commits Feb 19, 2018

Set the width of the parent element in breadcrumb tests
Setting the width of the parent element of the breadcrumbs and then
explicitly calling "_resize" is enough to test the resizing behaviour.
This makes possible to remove the "setMaxWidth" method and its related
code, which was used only for testing purposes.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Replace attribute with local variable
The "usedWidth" attribute was not used elsewhere outside the "_resize"
method, so it was replaced with a local variable. Moreover, it was also
renamed to a more suitable name ("availableWidth").

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Do not render the breadcrumbs again in resize tests
There is no need to call "setDirectory" again in resize tests; it is
enough to simply resize them (and isolates them better to just test the
resizing behaviour).

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Take padding and margin of the creatable actions div into account
There are some differences in width handling between the browsers used
to run the tests, most likely due to their support (or lack of) of
certain CSS features: PhantomJS requires "width" to be set (probably
because it does not handle flex displays and treats it like a block, so
"min-width" does not matter in this case), while Firefox requires
"min-width" to be set (otherwise the children of "#controls" could be
compressed due to its use of flex display and the elements would end
with a different width than the one needed for the tests). Due to all
that the width of the breadcrumb siblings must be specified in the tests
using both "width" and "min-width".

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Take all visible siblings into account
Other apps could add elements to the controls outside the creatable
actions div (for example, the button to switch to the gallery), so the
widths of all the visible siblings of the breadcrumbs have to be taken
into account in the size calculations.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Compress siblings before calculating the available width for crumbs
When the parent element of the breadcrumbs was resized to a larger width
and the siblings of the breadcrumbs expanded to fill all the available
width some crumbs could be hidden even if there was enough room for
them. The reason was that the width of the siblings being used to
calculate the available width for the breadcrumbs was the expanded width
of the siblings. Now as many crumbs as possible (that is, fitting in the
parent, no matter the siblings) are first shown so the expanding
siblings are compressed before calculating the available width.

Due to the lack of support for flexboxes in PhantomJS the related unit
test is skipped; it has to be run in other browser, like Firefox.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Use hard-coded values for paddings and margins
This ensures that the resize tests do not depend on the values set in
the CSS files.

Note that this change causes a test to fail with Firefox, but not with
PhantomJS. This is due to a difference in the starting width used by
Firefox and by PhantomJS, and it will be fixed in a following commit.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Take padding and margins of crumbs into account
When calculating the total width of the crumbs only its padding was
taken into account; now the margin is too. In a similar way, before
showing a crumb only its width was taken into account; now its padding
and margin are taken into account too.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Fix menu visibility
The crumb for the menu was shown like any other crumb when calling
"_showCrumb", but it was also shown when other crumbs were hidden
without taking into account the available width. This caused several
related problems, like the breadcrumbs taking too much space when the
menu was sometimes shown after the rest of the crumbs were adjusted to
the available width, or the menu being shown instead of the last crumb
even if there was room for it when the available width was increased.

Now the menu is always hidden before starting the resizing of the crumbs
to ensure that whether it was previously shown or not does not affect
the result. In a similar way, the menu will no longer be shown by
"_showCrumb", as it is not a regular crumb that has to be shown simply
if there is enough room. The menu is now shown as soon as any other
crumb is hidden; this ensures that the menu width will be taken into
account in further width checks. As when _updateMenu" is called it no
longer needs to take care of showing the menu this fixes the issue
revealed when fixing the test setup in the previous commit.

Finally, this implicitly fixes the failure in the breadcrumbs tests when
run on Firefox, as it was caused by the menu interfering in the
calculations of the other crumbs when increasing the width.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Fix setup to test the breadcrumbs menu
The "Shows only items not in the breadcrumb" test was failing when run
on Firefox, but not on PhantomJS. This was caused by the differences in
the starting width between both browsers and an incorrect setup of the
test (the width set for the crumbs was overriden when the breadcrumbs
were rendered again, and the breadcrumb was resized to 300 from an
indeterminate initial width).

Now the crumbs are rendered and then its width, padding and margin are
set to a known value. Then it is resized to 1000px, which ensures that
there will be enough room for all the crumbs and thus the menu will be
hidden, and finally it is resized to 300, which causes the middle crumb
to be hidden and the menu to be shown.

Note, however, that the test now always fails, no matter if it is run on
PhantomJS or on Firefox; if the menu crumb is hidden when "_updateMenu"
is called it will show it, but it will also wrongly try to add the menu
itself to the menu. As the "crumb-id" of the menu crumb is "-1" this
causes the last regular crumb to be added to the menu. This will be
fixed with other related issues in the next commit.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Do not show the crumbs again after hiding them
After the changes in the previous commit "_showCrumb" no longer shows
the menu, only the same crumb that was hidden by the last call to
"_hideCrumb". Therefore, if the crumb was hidden because it did not fit
there is no need to try to show it again, as it will still not fit.

Moreover, the calculated width for a hidden element is not always
accurate; in some cases the calculated width is lower than the actual
width (it happens, for example, when using a background image like the
"Share" icon), which causea the crumb to be shown even if there is not
enough room, which in the end causes the siblings to overflow the
contents.

No unit tests for this one, though; you will have to trust me on this,
sorry ;-)

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Improve documentation of "getTotalWidth"
"getTotalWidth" is not more accurate; it is simply not clamped.
Moreover, "width/outerWidth" could be used in tests too, and also even
if "getTotalWidth" could be used in tests while others not that would
not be something to be stated in the API documentation, but in a
comment.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Update comments in tests
Menu and home are not always visible; home is always visible, but menu
is shown only when needed.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>

@danxuliu danxuliu added this to the Nextcloud 13.0.1 milestone Mar 1, 2018

@danxuliu danxuliu requested review from rullzer, MorrisJobke and skjnldsv Mar 1, 2018

@codecov

This comment has been minimized.

codecov bot commented Mar 1, 2018

Codecov Report

Merging #8610 into stable13 will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             stable13    #8610   +/-   ##
===========================================
  Coverage       51.22%   51.22%           
  Complexity      24992    24992           
===========================================
  Files            1608     1608           
  Lines           95126    95126           
  Branches         1376     1376           
===========================================
  Hits            48726    48726           
  Misses          46400    46400
Impacted Files Coverage Δ Complexity Δ
apps/files_trashbin/lib/Trashbin.php 72.28% <0%> (-0.25%) 136% <0%> (ø)
lib/private/Files/Cache/Propagator.php 96.2% <0%> (+1.26%) 16% <0%> (ø) ⬇️
@skjnldsv

Okay here!

@rullzer

rullzer approved these changes Mar 5, 2018

@rullzer rullzer merged commit 655e39f into stable13 Mar 5, 2018

4 checks passed

codecov/patch Coverage not affected when comparing a2672a2...22f9d05
Details
codecov/project 51.22% remains the same compared to a2672a2
Details
continuous-integration/drone/pr the build was successful
Details
continuous-integration/drone/push the build was successful
Details

@rullzer rullzer deleted the stable13-8588-fix-breadcrumbs-width-calculation branch Mar 5, 2018

@MorrisJobke MorrisJobke referenced this pull request Mar 9, 2018

Merged

13.0.1 RC1 #8748

10 of 10 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment