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

Widgets work with both ~users and Teams #112

Closed
wants to merge 44 commits into from
Closed

Conversation

mattbk
Copy link
Contributor

@mattbk mattbk commented Feb 7, 2016

I took a crack at aligning the default widget code with the new public.json output from Gratipay.com. Comments are welcome; I'm sure there are ways to break what I've done.

Here’s a hack to make the link work.

Closes #109.

Also added a team widget example for debugging.
public.json structure has changed since these data were created.
Closes #111.

Not sure if there is a use case for anonymous anymore or not.

Also fixed bad quotation mark.
@mattbk mattbk added this to the Widgets 2.0 milestone Feb 7, 2016
@chadwhitacre
Copy link
Contributor

Nice! I've made a Review label in grtp.co so we could apply it here. :)

Anyone up for reviewing this? If not I'll get to it when I can ...

@mattbk
Copy link
Contributor Author

mattbk commented Feb 8, 2016

Should I worry about these checks failing?

@chadwhitacre
Copy link
Contributor

@mattbk Yes. It looks like our linter ain't happy with your formatting. ;-)

@mattbk
Copy link
Contributor Author

mattbk commented Feb 8, 2016

Okay, I see it now.

lib/v1/api.js
131 | text('receiving', (data.taking)? '$' + data.taking : '$' + data.receiving);
^ [W015] Expected 'text' to have an indentation at 17 instead at 21.
141 | }
^ [W015] Expected '}' to have an indentation at 17 instead at 21.

@mattbk mattbk self-assigned this Feb 9, 2016
@mattbk
Copy link
Contributor Author

mattbk commented Feb 9, 2016

I think tests are based on the old public.json and are failing because I downloaded new versions of the test public.json files?

@mattbk
Copy link
Contributor Author

mattbk commented Feb 10, 2016

Looking at these files in order?

Files: test/test_custom-widget-legacy.js,
test/test_custom-widget.js,
test/test_default-widget-anonymous.js,
test/test_default-widget-legacy.js,
test/test_default-widget.js,
test/test_expose-api-to-iframe.js,
test/test_giving-widget-anonymous.js,
test/test_giving-widget-legacy.js,
test/test_giving-widget.js -> test

@mattbk
Copy link
Contributor Author

mattbk commented Feb 10, 2016

I think it's failing on this part of api.js for certain tests.

  // Set up links and username before requesting public.json
            link('profile-link', gratipayURI + encodeURIComponent(username) + '/');
            link('link', gratipayURI);
            text('username', username);
            text('identity', 'I');

@mattbk
Copy link
Contributor Author

mattbk commented Feb 11, 2016

That works. Hot.

@chadwhitacre
Copy link
Contributor

/me waiting for Travis to run green ...

This makes test results easier to read.
@mattbk
Copy link
Contributor Author

mattbk commented Feb 11, 2016

/me waiting for Travis to run green ...

Hold your horses, I have to understand how tests work first.

@chadwhitacre
Copy link
Contributor

!m @mattbk

@mattbk
Copy link
Contributor Author

mattbk commented Feb 11, 2016

It looks like only giving can be anonymous now, not receiving. That doesn't help my brain work better, though.

There is no more anonymous receiving, only anonymous giving.
public.json now returns "null" rather than "anonymous".
This reverts commit 1b2d3af.
Not sure what this line does, but making sure this is the kicker.
@mattbk
Copy link
Contributor Author

mattbk commented Apr 2, 2016

I ended up merging trailing-zero into this branch because this is what it was based on. Next up is a full review by me of what I actually did.

@mattbk
Copy link
Contributor Author

mattbk commented Apr 2, 2016

I'm satisfied with the final product; it's a heck of a mess of commits, though. 🐺

@whit537, @techtonik, any comments?

@aandis
Copy link

aandis commented Apr 3, 2016

it's a heck of a mess of commits

Github now allows squash and merge commits! 💃


// If username exists, it's a ~user, not a team. Build URL accordingly.
if (data.username) {
link('profile-link', gratipayURI + encodeURIComponent(data.username) + '/');
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we want to insert a ~ here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure how those rewrites end up working. I know that if I don't put a ~ in, it still works. If it is better to include the ~, then we can do that.

@chadwhitacre
Copy link
Contributor

As I read it, this PR tries to treat the data-gratipay-username of widgets-in-the-wild as either a ~user username or a Team slug.

  1. If it's a username, we use that to construct the profile link, and expect that receiving won't be present but taking will be.
  2. If it's a slug, we use that to construct the profile link, and expect receiving to be present.

I'm not sure that's the right approach to take, both because it introduces tech debt into our widget code ("receiving" in this PR no longer means "receiving," it means "receiving or taking"), and because it's not as true to our new data model as it could be. I think a better approach will be to let data-gratipay-username continue to only mean ~user username, and write new widget code for Teams. See #103 (comment) for further discussion.

I propose that we whittle this PR down to just the test changes that make sense to keep, and pick up with additional changes in new PRs.

@mattbk
Copy link
Contributor Author

mattbk commented Apr 11, 2016

I propose that we whittle this PR down to just the test changes that make sense to keep, and pick up with additional changes in new PRs.

I can work on that, unless someone else wants to jump in.

@mattbk
Copy link
Contributor Author

mattbk commented Apr 20, 2016

just the test changes

I might need clarification. You mean make this PR about ~users and build a new PR for teams?

@chadwhitacre
Copy link
Contributor

I actually meant to keep the extra info in the test strings like below, and make new PRs for ~user and Team changes.

screen shot 2016-04-20 at 11 27 06 am

@mattbk
Copy link
Contributor Author

mattbk commented Apr 20, 2016

Got it. Now I get to learn how to clone my PR without losing things.

mattbk added a commit that referenced this pull request Apr 22, 2016
Based on discussion at
#112 (comment).
Breaking up that PR into more manageable chunks.
This was referenced Apr 22, 2016
chadwhitacre pushed a commit that referenced this pull request Apr 27, 2016
Based on discussion at
#112 (comment).
Breaking up that PR into more manageable chunks.
@mattbk
Copy link
Contributor Author

mattbk commented May 13, 2016

Shuttering this pull request because it changes v1 widgets, which is not the plan at #103 (comment).

Keeping the branch in place until I grab what I want for a real v2.

@mattbk mattbk closed this May 13, 2016
@mattbk mattbk removed the Review label May 13, 2016
@mattbk mattbk mentioned this pull request Jun 19, 2016
5 tasks
@chadwhitacre chadwhitacre deleted the team-experiments branch December 28, 2016 16:33
@chadwhitacre chadwhitacre restored the team-experiments branch December 28, 2016 16:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants