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

Add tooltip for longer card names #3278

Merged
merged 3 commits into from
Mar 9, 2018

Conversation

mrpau-eugene
Copy link
Contributor

@mrpau-eugene mrpau-eugene commented Mar 1, 2018

Summary

Still need some improvements and would also like some suggestions if there would be better approach for showing the tooltip.

Before:

screen shot 2018-03-01 at 3 16 07 pm

After:

screen shot 2018-03-01 at 3 15 47 pm

There's a small problem after wrapping the <router-link> with a <span> (though I'm not quite sure if this is the best practice for adding tooltips) since it's causing a problem with the bottom margin for the content cards.

Submitting this PR for asking other suggestions and also whether to know if I'm on the right path 🙈

Reviewer guidance

References

#2193


Contributor Checklist

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • Contributor has fully tested the PR manually
  • Screenshots of any front-end changes are in the PR description
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')

Reviewer Checklist

  • Automated test coverage is satisfactory
  • Reviewer has fully tested the PR manually
  • PR has been tested for accessibility regressions
  • External dependencies files were updated (yarn and pip)
  • Documentation is updated
  • Link to diff of internal dependency change is included
  • CHANGELOG.rst is updated for high-level changes
  • Contributor is in AUTHORS.rst

@mrpau-eugene mrpau-eugene added the work-in-progress Not ready for review label Mar 1, 2018
@codecov
Copy link

codecov bot commented Mar 1, 2018

Codecov Report

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

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #3278      +/-   ##
===========================================
+ Coverage    74.44%   74.57%   +0.13%     
===========================================
  Files          209      209              
  Lines         8229     8221       -8     
  Branches      1004     1001       -3     
===========================================
+ Hits          6126     6131       +5     
+ Misses        1902     1889      -13     
  Partials       201      201
Impacted Files Coverage Δ
...tend_build/src/prettier-frontend-webpack-plugin.js 33.33% <0%> (+23.33%) ⬆️
frontend_build/src/webpack.config.base.js 100% <0%> (+25%) ⬆️

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 b331da8...c2dbf6b. Read the comment docs.

@indirectlylit
Copy link
Contributor

Hi @mrpau-eugene

Looks like a good start!

Two quick points:

  • I'm not actually able to see the tooltip
  • We should be able to show three lines of text. The old version (almost) did, but the new version does not:
old new
image image

I'll check out the code and experiment a little now.

@indirectlylit
Copy link
Contributor

indirectlylit commented Mar 2, 2018

It looks like the ui-tooltip component doesn't handle large text strings too well.

Instead we should probably just use the native tooltip.

ui-tooltip native
image image

@indirectlylit
Copy link
Contributor

submitted a PR here with some possible changes corresponding to the comments I left above: mrpau#1

These are just options - recommend reading carefully and deciding if you like them.

@indirectlylit
Copy link
Contributor

I'd also look carefully at the performance here. After some brief testing, I believe that the vue-shave directive is updating all of these DOM elements FAR too often based on global resize events: https://github.com/quantity-digital/vue-shave/blob/master/src/vue-shave.js

I'm thinking we should remove vue-shave and instead write our own wrapper around shave which leverages our responsive-element mixin to decide when to run the shavers, since we can more efficiently listening for element-specific resize events using that: https://github.com/learningequality/kolibri/blob/develop/kolibri/core/assets/src/mixins/responsive-element.js

@indirectlylit
Copy link
Contributor

This isn't working yet, but I'd recommend creating a new shaved-text component that is similar to this:

https://gist.github.com/indirectlylit/dea91635bf71974334c3f02ee120f29a

then we can make it a core component and re-use it in other places.

@mrpau-eugene
Copy link
Contributor Author

I'm thinking we should remove vue-shave and instead write our own wrapper around shave which leverages our responsive-element mixin to decide when to run the shavers, since we can more efficiently listening for element-specific resize events using that

+1 for this!

This isn't working yet, but I'd recommend creating a new shaved-text component that is similar to this

Will do this one. Should I place it somewhere in kolibri/core/asserts/views directory and make something similar like the k-select, k-checkbox, etc.. ?

@indirectlylit
Copy link
Contributor

Will do this one. Should I place it somewhere in kolibri/core/asserts/views directory and make something similar like the k-select, k-checkbox, etc.. ?

For now, just put it at content-card/shaved-text.vue. Once it's done we can move it to core.

@indirectlylit
Copy link
Contributor

indirectlylit commented Mar 8, 2018

this looks great - nice job!

@indirectlylit indirectlylit added this to the 0.9.0 milestone Mar 8, 2018
@indirectlylit indirectlylit merged commit 9cca9a4 into learningequality:develop Mar 9, 2018
@benjaoming benjaoming mentioned this pull request May 28, 2018
14 tasks
@benjaoming benjaoming mentioned this pull request Oct 16, 2018
14 tasks
@cpauya cpauya deleted the bugfix/content-cards branch May 21, 2019 02:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
work-in-progress Not ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants