Skip to content
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

Fix file header text wrapping on long paths on Bitbucket #11998

Closed
wants to merge 2 commits into from

Conversation

marekweb
Copy link
Contributor

@marekweb marekweb commented Jul 7, 2020

Fix #11110

Summary of attempts to solve this issue:

  • Didn't find a way to keep using display flex while making the file path wrap
  • No clear solution to wrap both file path and toolbar buttons: because they're on the same line, the desired wrapping behavior isn't obvious
  • Simplest apparent solution: remove flex and use aui-buttons class which already exists and is used on Bitbucket toolbar buttons.

Limitations of this solution:

  • Toolbar buttons don't wrap if there's an excessive number of them. The file path (sharing space with the buttons) will wrap instead.

@marekweb marekweb requested a review from a team July 7, 2020 16:39
@codecov
Copy link

codecov bot commented Jul 7, 2020

Codecov Report

Merging #11998 into master will decrease coverage by 2.23%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #11998      +/-   ##
==========================================
- Coverage   49.95%   47.71%   -2.24%     
==========================================
  Files        1517     1412     -105     
  Lines       88450    80191    -8259     
  Branches     6658     6712      +54     
==========================================
- Hits        44182    38267    -5915     
+ Misses      40339    38346    -1993     
+ Partials     3929     3578     -351     
Flag Coverage Δ
#go 51.91% <ø> (-2.45%) ⬇️
#storybook 10.74% <0.00%> (-0.01%) ⬇️
#typescript 36.58% <100.00%> (+<0.01%) ⬆️
#unit 47.29% <100.00%> (-2.28%) ⬇️
Impacted Files Coverage Δ
...owser/src/shared/code-hosts/bitbucket/codeHost.tsx 61.44% <100.00%> (+0.47%) ⬆️
cmd/frontend/db/orgs_mock.go 0.00% <0.00%> (-100.00%) ⬇️
cmd/frontend/db/users_mock.go 0.00% <0.00%> (-100.00%) ⬇️
internal/extsvc/github/codehost.go 0.00% <0.00%> (-100.00%) ⬇️
internal/extsvc/gitlab/codehost.go 0.00% <0.00%> (-100.00%) ⬇️
...rker/internal/correlation/lsif/jsonlines/reader.go 0.00% <0.00%> (-100.00%) ⬇️
internal/authz/bitbucketserver/authz.go 0.00% <0.00%> (-89.48%) ⬇️
internal/metrics/operation.go 0.00% <0.00%> (-87.70%) ⬇️
internal/authz/gitlab/gitlab.go 0.00% <0.00%> (-86.52%) ⬇️
...e-code-intel-bundle-manager/internal/paths/util.go 0.00% <0.00%> (-83.34%) ⬇️
... and 261 more

@christinaforney
Copy link
Contributor

christinaforney commented Jul 7, 2020

Thanks for working on this and finding a simplified solution! Can you please add screenshots of what your solution looks like, in the normal, long, and breakingly long states (for record keeping)?

@felixfbecker
Copy link
Contributor

felixfbecker commented Jul 7, 2020

To make sure I understand correctly, this is basically a revert of #9522 ? I.e. we are reintroducing #4480 for Bitbucket?
If yes, it's probably better than the current state but still not great that we are exchanging one issue for another...

@felixfbecker felixfbecker added this to Needs review in Web Team :: Current iteration via automation Jul 7, 2020
Copy link
Contributor

@felixfbecker felixfbecker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it's EOD for me, approving the code since the code looks good to me. @christinaforney I'll let you review the screenshots once @marekweb posted them to assess whether this is an acceptable regression from a product perspective 🙂

Web Team :: Current iteration automation moved this from Needs review to Reviewer approved Jul 7, 2020
@christinaforney
Copy link
Contributor

@marekweb - can you please post some screenshots so I can review the regression?

@marekweb
Copy link
Contributor Author

I tested in a variety of configurations and I found that there are still wrapping issues. Screenshots are below.

As an additional change I disabled overflow: hidden on action buttons. That style isn't a problem in itself, but it was causing a mis-alignment of the action buttons relative to the native buttons (although that only happened when injecting action buttons, not when it's a single Sourcegraph button).

In narrow configurations I'm still not entirely happy with how the toolbar is wrapping, (it's breaking when combining ultra-narrow width with multiple large action buttons) and I'm also not entirely confident that removing overflow: hidden is the right solution. So far I haven't found an alternative that works and that combines proper wrapper of the long file breadcrumbs with proper wrapping of a larger number of action buttons.

Screen Shot 2020-07-14 at 11 51 23

Screen Shot 2020-07-14 at 14 07 08

Screen Shot 2020-07-14 at 11 53 10

Screen Shot 2020-07-14 at 11 53 33

@felixfbecker
Copy link
Contributor

@marekweb @christinaforney I hacked together a PoC of how the issue could be fixed while not regressing on the action item wrapping: #12182
@marekweb could you test if this works?

@felixfbecker felixfbecker added bitbucket Issues that occur with the Bitbucket Server code host browser-extension labels Jul 14, 2020
@marekweb
Copy link
Contributor Author

Closing in favor of #12182 which has been merged

@christinaforney
Copy link
Contributor

Closing as Merek notes in previous comment.

Web Team :: Current iteration automation moved this from Reviewer approved to Done Jul 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bitbucket Issues that occur with the Bitbucket Server code host browser-extension
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Bitbucket Server native integration prevents file header text from wrapping
3 participants