-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
14731 plugins catalog #16763
14731 plugins catalog #16763
Conversation
The screenshots look great, and I'm finally getting around to checking out the branch and taking it for a spin. At the moment, the plugins catalog is bombing out for me with:
That date expression is the value of the |
Seems like it would be a lot cleaner (and easier) to list the available plugins in a table rather than in the grid layout pictured above. A table provides more efficient use of space and enables easy ordering by column. It would also make it much easier to compare the installed version of each plugin to its latest release. |
I'm fine with a table layout for the initial implementation. We'll revisit the card / grid design down the road, during or after the 4.1 beta period. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add some basic tests to validate the API processing and views?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice progress here. BTW the Markdown rendering in plugin.description_short
is now working as expected.
@jeffgdotorg Detail View changed quite a bit from Jeremy's feedback - please take a re-review: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple of small asks here. Otherwise looks good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple additional flags from talking through this PR with @natm
Added proxy header and user-agent string to API call. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @arthanson!
Fixes: #14731
Changed from the design of cards to a standard table as per comments. Held off on writing tests as it is info display only and it sounds like we want to change to cards fairly soon which would require re-writing a lot of the tests. We could mock the API and check the display based on that, but not sure what really to test beyond that.