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

#6754 truncatefy fix #6830

Merged
merged 10 commits into from
May 19, 2020
Merged

#6754 truncatefy fix #6830

merged 10 commits into from
May 19, 2020

Conversation

rationality6
Copy link
Contributor

@rationality6 rationality6 commented Apr 30, 2020

Summary

this is fix for number #6754

#6754

before
before

after

after

Added one div tag in KRadioButton for class

and add css for truncate texts

Contributor Checklist

PR process:

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Testing:

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

Reviewer Checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@codecov
Copy link

codecov bot commented Apr 30, 2020

Codecov Report

Merging #6830 into develop will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted Files Coverage Δ
packages/kolibri-components/src/KRadioButton.vue 100.00% <ø> (ø)
kolibri/core/content/utils/channel_import.py 88.92% <0.00%> (+0.56%) ⬆️
kolibri/core/tasks/utils.py 93.22% <0.00%> (+5.08%) ⬆️

@indirectlylit indirectlylit added this to the 0.14.0 milestone Apr 30, 2020
@indirectlylit
Copy link
Contributor

It looks the div is a sibling of a span, which is not typical because one is block and the other is inline-block. I would expect siblings to use the same display property.

@rationality6
Copy link
Contributor Author

rationality6 commented May 2, 2020

@indirectlylit Is the way that Kolibri code intends? Let me know if I've misunderstood. Then I'll fix again.

@indirectlylit
Copy link
Contributor

I'm still a bit confused - now it's two inline blocks with width 100%?

image

My guess is that both those spans (for the label and description) should actually just be divs with their default display: block properties?

@indirectlylit
Copy link
Contributor

thanks @rationality6

code looks good to me. @nucleogenesis would you mind testing? Also flagging in learningequality/kolibri-design-system#12

@rationality6
Copy link
Contributor Author

I forgot to comment on code after commit, sorry. Thank you for reviewing @indirectlylit.

@nucleogenesis
Copy link
Member

nucleogenesis commented May 12, 2020

@indirectlylit @rationality6

I'm not getting the ellipses - rather the text is wrapping in its container. Tested in Firefox and Chromium.

long-name-network-source-kolibri

I can get the overflow: ellipsis to work if I add the white-space: nowrap; CSS property to the text-truncate class though. However, I do think that wrapping looks good. There is a 40 character limit to the name of the network when added and named through this dialog (seen in the above example) - and even if network names could be longer by some other means, it looks fine enough IMO. See the below image for an exaggerated example of a very very very long network name.

extra-super-long-name-network-source-kolibri

@rationality6
Copy link
Contributor Author

rationality6 commented May 12, 2020

Thank you @nucleogenesis. I found out what I made a mistake. "White-space: nowrap;" as you say
. If there is a whitespace in String, it will have new line Or it works the way I thought it would.
Once I added "white-space: nowrap;" I think it's okay to just change the line, as you said. Let me know, If the way would be better.

Screen Shot 2020-05-12 at 12 48 31 PM

after with nowrap

Screen Shot 2020-05-12 at 12 59 54 PM

@rationality6
Copy link
Contributor Author

rationality6 commented May 14, 2020

@nucleogenesis, @indirectlylit
Do you guys know what am I missed? I just put in a single line of css.

@nucleogenesis
Copy link
Member

Hey @rationality6 that looks good to me so far.

As for the failing Buildkite, there were some issues with the build system. I just restarted it so it will likely pass and be ready for merge. I'll check back on it and merge when passing.

Thank you @rationality6 !!!

Copy link
Contributor

@indirectlylit indirectlylit left a comment

Choose a reason for hiding this comment

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

thank you!

@indirectlylit indirectlylit merged commit f94acb3 into learningequality:develop May 19, 2020
@rationality6 rationality6 deleted the text_truncate_fix branch June 3, 2020 05:21
indirectlylit added a commit to indirectlylit/kolibri-design-system that referenced this pull request Aug 4, 2020
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.

4 participants