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

Create Truncation Component #1500

Merged
merged 45 commits into from Jun 5, 2020
Merged

Create Truncation Component #1500

merged 45 commits into from Jun 5, 2020

Conversation

seve
Copy link
Member

@seve seve commented May 28, 2020

Across the app, we practice truncating large strings by taking a slice of some size out of the middle of the string and replacing it with ellipses. The number of truncations needed throughout the app has begun to grow and the work associated with keeping the methodology between instances has been growing with it.

This PR introduces a Truncation component in the new dir: client/src/components/util/. Given a single child holding text and a width style, the component calculates whether the text needs truncation and truncates to the largest fitting size.

h/t @tihuan for providing a CSS solution and steering me along the way 🎉


This PR implements the following changes:

  • client/src/components/util/truncate.js
    • Created the truncate component
  • client/src/components/categorical/category/index.js + client/src/components/categorical/value/index.js + client/src/components/leftSidebar/topLeftLogoAndTitle.js
    • Refactor in Truncate component
  • client/src/components/graph/overlays/centroidLabels.js + client/src/components/categorical/value/occupancy.js
    • Remove fontFamily style declarations
  • client/configuration/eslint/eslint.js
    • Removal of redundant/conflicting manual ESLint rules
  • client/configuration/lint-staged/lint-staged.config.js
    • Correctly lint only staged files instead of all of src

Closes #1489

@codecov-commenter
Copy link

codecov-commenter commented May 28, 2020

Codecov Report

Merging #1500 into master will increase coverage by 0.80%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1500      +/-   ##
==========================================
+ Coverage   60.81%   61.62%   +0.80%     
==========================================
  Files          35       36       +1     
  Lines        3325     3528     +203     
==========================================
+ Hits         2022     2174     +152     
- Misses       1303     1354      +51     
Flag Coverage Δ
#backend 61.62% <ø> (+0.80%) ⬆️
#python 61.62% <ø> (+0.80%) ⬆️
#unitTest 61.62% <ø> (+0.80%) ⬆️
Impacted Files Coverage Δ
server/compute/diffexp_cxg.py 84.31% <0.00%> (-8.79%) ⬇️
server/data_common/data_adaptor.py 76.96% <0.00%> (ø)
server/converters/to_sparse.py 0.00% <0.00%> (ø)
server/data_common/fbs/NetEncoding/Matrix.py 84.72% <0.00%> (+1.38%) ⬆️
server/converters/cxgtool.py 82.97% <0.00%> (+5.05%) ⬆️
server/data_cxg/cxg_adaptor.py 49.19% <0.00%> (+5.31%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 030eea1...5b14b61. Read the comment docs.

@seve seve requested review from colinmegill, mweiden and tihuan and removed request for mweiden May 28, 2020 23:35
@tihuan
Copy link
Contributor

tihuan commented Jun 1, 2020

Ahh thanks for testing the 5 + 5 case! Yeah the gap is really huge 😆 Definitely not good

Screen Shot 2020-06-01 at 2 50 26 PM

What's your thought on this Firefox discussion board's solution?

I added the same test cases from the Stackoverflow one, and it seems to work better:

Screen Shot 2020-06-01 at 2 49 46 PM

https://codepen.io/tihuan/pen/qBbWoog

And the only hard coded value is min-width: 30px;!

.split {
  width: 300px;
  background: pink;
  display: flex;
  overflow: hidden;
  justify-content: flex-start;
}

.first {
  overflow: hidden;
  text-overflow: ellipsis;
  white-space: nowrap;
  flex-shrink: 1;
  min-width: 30px;
}

.second > span {
  position: absolute;
  right: 0;
  color: initial;
}
.second {
  color: transparent;
  position: relative;
  overflow: hidden;
  white-space:nowrap;
}

PTAL 🎉 !

UPDATE: Changed the width to 250px from 300px to see more truncations:

Screen Shot 2020-06-01 at 2 53 18 PM

@seve
Copy link
Member Author

seve commented Jun 1, 2020

@tihuan I think that's the one! I'm going to run it against some other datasets, but its looking good so far!
image

@tihuan
Copy link
Contributor

tihuan commented Jun 1, 2020

So glad to hear that 😭 !! Thanks for your patience on this one, @seve !!

If that works for our cases, I'll share the code and credit the author back to the original Stackoverflow solution, so other people can see it too 😆

@tihuan
Copy link
Contributor

tihuan commented Jun 2, 2020

Woot woot go @seve !! 🎉 🎉 🎉

Thanks so much for testing this approach thoroughly in the app! (I finally set up cellxgene with Matt's help this morning lol)

I'll share the solution back on Stackoverflow now too!

Copy link
Contributor

@tihuan tihuan left a comment

Choose a reason for hiding this comment

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

Looks super good! Just a few nits. Otherwise LGTM! Thanks again for taking on this, @seve 🎉 👍 🥇

client/src/components/util/truncate.js Show resolved Hide resolved
client/src/components/util/truncate.js Outdated Show resolved Hide resolved
client/src/components/util/truncate.js Outdated Show resolved Hide resolved
client/configuration/eslint/eslint.js Show resolved Hide resolved
client/src/components/categorical/value/index.js Outdated Show resolved Hide resolved
client/src/components/leftSidebar/topLeftLogoAndTitle.js Outdated Show resolved Hide resolved
Copy link
Contributor

@bkmartinjr bkmartinjr left a comment

Choose a reason for hiding this comment

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

several nits/cleanup items, but OK overall. Thanks!

Copy link
Contributor

@bkmartinjr bkmartinjr left a comment

Choose a reason for hiding this comment

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

ps. please don't merge until all tests pass.

@seve seve merged commit 4e96847 into master Jun 5, 2020
@tihuan tihuan deleted the seve/component-truncation branch June 5, 2020 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consolidate text truncation into a unified component
5 participants