Up for review! :boom:

Known issues:
- The tables' column widths go haywire when there are no rows in the table. This happens on the My issues page as well.
- The No pull-request found message is not displayed in certain views. For example, even if there are no open PRs, the message is not displayed in the Open view because the rows for other views are actually present, but hidden unless it's the Closed view or All view.

I can fix this with Javascript, but I'm not sure if that's the way to go, or if we should somehow do it in Flask / Jinja itself. Please suggest :)

can you post a screenshot of the changes you made :)

The tables' column widths go haywire when there are no rows in the table.

I'm seeing the headers of the table be of different length, but since the table is empty it doesn't seem that bad to me, so not sure if I'm seeing the same thing as you (ie: I'd not call it haywire)

The No pull-request found message is not displayed in certain views.

It seemed to display here, when did it not?

can you post a screenshot of the changes you made :)

Sure!

How it was earlier: https://paste.opensuse.org/view/simple/20b80768

How it is now:
- https://paste.opensuse.org/view/simple/5eb61eea
- https://paste.opensuse.org/view/simple/ef723db6

I'm seeing the headers of the table be of different length

This is what I meant. It'd be nice to have uniform column widths for the corresponding columns in both tables, which are consistently displayed even when the tables are empty. But I suppose I am just trying to make it pretty :)

It seemed to display here, when did it not?

The No pull-request found message only displays when there are zero pull requests in either of the tables. It won't display, for instance, in the following cases:

• If there is one open PR in the Opened PRs table and one merged PR in the Other PRs table, there is no message in the second table when seeing the Open view.

• If there is one merged PR in the Opened PRs table and one open PR in the Other PRs table, there is no message in the first table when seeing the Open view.

These can be seen in the screenshots linked above.

This happens because we are using a for-else construct in the template, and the else block is never triggered if there is atleast one row in the table (whether its visible or hidden), which is what happens in the examples above.

In my humble opinion, this doesn't look nice. There may be another option -> Pull Requests to 's :P

I like the idea that there should be another option 'Merged' at top-left in addition to present options All, Open and Closed in /pull-requests.

There may be another option -> Pull Requests to 's

I'm not sure I follow you there, does that mean that if you have 100 repos the drop-down menu contain 100 entries?

There may be another option -> Pull Requests to 's

I'm not sure I follow you there, does that mean that if you have 100 repos the drop-down menu contain 100 entries?

Nopes, according to the fix, page shows pull request by user A in the projects and other user's pull requests on same project on the same page.

I think other user's pull requests list must on be other page

You seem to imply that this page lists PR of a certain repo, but you did notice that this page isn't linked to any repo? It is the list of PRs you opened and PRs on which you can act (ie: merge/close) so it concerns all the repo to which you have rights and have worked.

ok , understood :sweat_smile:

@cep what do you think of the following:

@pingou: A few comments / questions:

• The indentations in the source code still appear to be inconsistent. I will take a look.

• It looks like you're doing {{ visible.append(true) }}. Since this is essentially a print statement, is it okay to do it this way?

• The :not(:hidden) selector can be quiet expensive. Perhaps we could use .not() or .filter(':visible').length > 0 instead?

  Relevant: http://stackoverflow.com/a/17426800/3186769

Thanks for the ideas, I will push a patch shortly :)

Feel free to apply the patch and build on the top of it with one fixing it :)

Done!

@pingou I couldn't get the raw patch file, so I just made the changes you mentioned and polished it up a bit.

@pingou I'm not convinced we should say PRs for foo here, because we already display PRs for foo at the top. Also, I feel the avatar and username are not needed here, and clutter the view a bit.

Comment on line 13 actually goes here. Oops :grin:

Well, when I was testing this PR I ended up wondering what was the difference between the two tables, so while I'm not set on the wording I do think we should be more explicit about what is presented to the user.

Ah, I understand. In that case, let's go with this for now, considering this is more explicit / clear. We can clean it up later if it annoys a few too many people :) That ok?

That's ok with me :)

Alright, let's merge :)