Skip to content
This repository was archived by the owner on May 12, 2021. It is now read-only.

Add super-short bio summary to mouseover popup. #1277

Merged
black-puppydog merged 3 commits into from
May 5, 2020
Merged

Add super-short bio summary to mouseover popup. #1277

black-puppydog merged 3 commits into from
May 5, 2020

Conversation

@black-puppydog
Copy link
Contributor

@black-puppydog black-puppydog commented Apr 23, 2020

This addresses #780 in a relatively hap-hazard way.
I basically just tried to copy what the code around this feature was doing.
Also I didn't know any other way to ditch markdown syntax than to include a new dependency.

Alternatively we could just pass the raw markdown, and hope that users will notice that it doesn't look so great, and just have super-short bios in their profiles' first lines. 😛

@black-puppydog
Copy link
Contributor Author

black-puppydog commented Apr 24, 2020

@christianbundy have you had trouble like this with travis before?

@christianbundy
Copy link
Contributor

christianbundy commented Apr 24, 2020

Yeah, usually after a few hours you can restart the build and they'll go green again. 🙄

I tried one restart a couple of hours ago, I'll try again later today.

@black-puppydog black-puppydog force-pushed the mouseover-bio branch 2 times, most recently from e54e213 to 6a34aa5 Compare April 24, 2020 19:57
@black-puppydog
Copy link
Contributor Author

black-puppydog commented Apr 24, 2020 •
edited
Loading

Okay, best I can tell, there's an issue with how we handle caching in travis.

  • Caching ~/.npm/ explicitely doesn't seem to be the recommended way anymore.
  • It works when I disable caching entirely.
  • It fails again if I re-enable it and it uses the caches it was using before because the uncached build didn''t modify those.
  • It works if I put npm cache verify into the before_install hook, which produces some interesting retults:
    • On Linux (which was failing before) it shows this: Corrupted content removed: 1
    • On Windows, caching seems to fail: Content verified: 0 (0 bytes). After the build, uploading the cache seems to fail, which would explain that. failed to upload cache is not a reason for travis to fail the build though.

If nobody else wants to have a go at this, I'd say we leave it like this for now, at least if it breaks now we might get more pointers as to what's wrong.

@black-puppydog
Copy link
Contributor Author

black-puppydog commented Apr 24, 2020

So, travis problems notwithstanding, I'm still unsure about the quality of this PR and would like input. :P

@christianbundy
Copy link
Contributor

christianbundy commented Apr 24, 2020

I'd be happy to help, but I think I need to take a few days away from Patchwork and try to avoid burnout. :/ I'm happy to look at this next week though, or if anyone else has energy then please feel free to merge it.

@black-puppydog
Copy link
Contributor Author

black-puppydog commented Apr 24, 2020 •
edited
Loading

Yes, of course Christian! This is by no means urgent, so you just make sure you're good before touching this with a ten foot pole. More important now than ever. :)

Edit: sorry for the mess down there ⬇️ I kinda re-arranged the commit tree there and got tangled in it. 😆

black-puppydog added a commit that referenced this pull request Apr 24, 2020
@black-puppydog
Handle travis cache more delicately.
089ed8a 
After debugging in #1277, it seems that our travis cache got corrupted.
This simply puts a cache verification step in front of the build process.
If the caching becomes a problem now, we shoul at least get some clear
info on what's happening.

Also, this disables explicit caching of ~/.npm, because that is handled
implicitly by travis these days.
black-puppydog added a commit that referenced this pull request Apr 24, 2020
@black-puppydog
Handle travis cache more delicately.
1541579 
After debugging in #1277, it seems that our travis cache got corrupted.
This simply puts a cache verification step in front of the build process.
If the caching becomes a problem now, we shoul at least get some clear
info on what's happening.

Also, this disables explicit caching of ~/.npm, because that is handled
implicitly by travis these days.
black-puppydog added a commit that referenced this pull request Apr 24, 2020
@black-puppydog
Handle travis cache more delicately.
83e7340 
After debugging in #1277, it seems that our travis cache got corrupted.
This simply puts a cache verification step in front of the build process.
If the caching becomes a problem now, we shoul at least get some clear
info on what's happening.

Also, this disables explicit caching of ~/.npm, because that is handled
implicitly by travis these days.
This was referenced Apr 24, 2020
Merged
Closed
Powersource
Powersource reviewed Apr 27, 2020
View reviewed changes
lib/depject/profile/html/preview.js Outdated
const when = require('mutant/when')
const computed = require('mutant/computed')
const send = require('mutant/send')
const removeMd = require('remove-markdown')
Copy link
Contributor

@Powersource Powersource Apr 27, 2020

Choose a reason for hiding this comment

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

Instead of this, and shortenDescription, I think you should be able to use https://github.com/ssbc/patchwork/blob/master/lib/depject/message/async/name.js#L53 instead. It's what we use to create the text for e.g. links to forks and references.

@black-puppydog
Add super-short bio summary to mouseover popup.
bb6489a 
This addresses #780 (Mouseover of profiles should include some of the
user bio) in a relatively hap-hazard way.
@black-puppydog black-puppydog force-pushed the mouseover-bio branch 2 times, most recently from 0b26fe4 to f9b5d6f Compare May 3, 2020 19:51
@black-puppydog
Remove remove-markdown dependency.
5c0503d 
We already had a method to make a short version of a markdown text.
It was stuck deep in a depject module, so I ripped it out of there and put
it into its own module.
It is now require()d in the depject module and in the new bio preview code.
@black-puppydog
Copy link
Contributor Author

black-puppydog commented May 3, 2020

Okay @Powersource, I think I have sth usable here. The markdown summary function was a helper in a depject module. But I read somewhere that everything depject is to be considered deprecated, so instead of working out how the f**k depject works, I thought I could just rip that out and require it from a new module.
Of course, I'm not sure if I put the new file in the right place. Open to suggestions here.
It works for me though, so...

@black-puppydog
Copy link
Contributor Author

black-puppydog commented May 5, 2020

Okay, one tipsy dev kinda found this okay. Has to be enough for now. Feel free to revert.

image

@black-puppydog black-puppydog merged commit 52ea11c into master May 5, 2020
@christianbundy
Copy link
Contributor

christianbundy commented May 5, 2020

instead of working out how the f**k depject works, I thought I could just rip that out and require it from a new module

+1

Okay, one tipsy dev kinda found this okay

+100

🚀

black-puppydog added a commit that referenced this pull request May 12, 2020
@black-puppydog
remove unused dependency from package-lock.json
9c8e6dd 
This slipped in in #1277. I removed the dependency from package.json
following @Powersource's recommendation but forgot to rebuild the lockfile.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@Powersource Powersource Powersource left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@black-puppydog @christianbundy @Powersource