Show more button on home page implementation #3424

jmuzsik · 2018-03-26T18:21:44Z

Hi, in reference to #1303. This is what I did:

Some things to mention:

If you look at the video you'll notice that there is sometimes this gimicky behaviour when I reload quickly. I tried a handful of ways to fix this but think the only real way is inline styling, setting display:none automatically when the package box is created but that is not allowed. This or something can be done in python in which case I have no clue where to even begin.
I added this little bit of logic: {% set summary = release.summary if release.summary else "No description provided" %} to get rid of the creation of an empty paragraph element that caused the boxes being even which you'll notice exists in the issue image.
Also, let me know if I should delete the if statements in show-more-initial because there will always be more than 20 packages in latest and trending. In the mock database there are sometimes only 19 for trending.
Last, basically all these frameworks are new to me so I am not sure if I am doing best practices.

Thanks!

drunkwcodes

I'm wondering where are these buttons at every time while browsing the home page.

LGTM.

brainwane · 2018-03-30T02:47:52Z

Thank you for the PR, @jmuzsik ! @yeraydiazdiaz , could you review?

yeraydiazdiaz

Thanks for your PR, it really helps the front page. I left a few comments, let me know if you need any clarification.

yeraydiazdiaz · 2018-03-30T16:47:32Z

warehouse/static/js/warehouse/utils/show-more-initial.js

+      latestElements[i].style.display = "none";
+    }
+  }
+};


Maybe this logic could live in the Stimulus controller? You could use data-target attributes instead of hide-by-index-* classes which I believe do not add styling and then write a connect method that does the hiding of the last 15 items. The password_controller uses a similar pattern.

Ok I figured out how to use Stimulus properly (I believe). I got rid of any manipulation of the DOM without using Stimulus

yeraydiazdiaz · 2018-03-30T16:49:08Z

warehouse/templates/index.html

@@ -84,35 +97,39 @@ <h1 class="homepage-banner__title">Find, install and publish Python packages wit
  </div>
 </section>

-<section class="horizontal-section horizontal-section--grey">
-  <div class="site-container">
+<section class="horizontal-section horizontal-section--grey" data-controller="increment">


increment seems a bit generic to me, show_more might be more applicable?

yeraydiazdiaz · 2018-03-30T16:51:54Z

warehouse/templates/index.html

    <div class="col-half">
      <div class="heading-wsubtitle">
        <h2 class="heading-wsubtitle__heading">Trending Projects</h2>
        <p class="heading-wsubtitle__subtitle">Trending projects as downloaded by the community</p>
      </div>
-      <div class="package-list">
+      <div class="package-list-timeout">


what does the -timeout suffix do? I can't find a reference to it

you're right, unnecessary

yeraydiazdiaz · 2018-03-30T17:02:27Z

warehouse/templates/index.html

+          {% if current.amount_to_show < trending_projects_visible.total %}
+            {{ project_snippet_trending(release, release.summary, current.amount_to_show) }}
+            {% set current.amount_to_show = current.amount_to_show + 1 %}
+          {% endif %}


this loop might be clearer simply iterating over an index? something like

{% for i in range([trending_projects_visible.total, trending_projects|length]|min) %} {{ project_snippet_trending(trending_projects[i], trending_projects[i].summary, i) }} {% endfor %}

(the range argument is admittedly awkward due to min and length being Jinja2 template filters)

made it much more clear now, actually how it was originally, I did not need all those extra jinga variables

yeraydiazdiaz · 2018-03-30T17:03:12Z

warehouse/templates/index.html

-  </h3>
-  <p class="package-snippet__description">{{ release.summary }}</p>
+{% set trending_projects_visible = namespace(total=20) %}
+{% set latest_releases_visible = namespace(total=20) %}


do you need the namespace? seems like it could simply be latest_releases_visible_total = 20

didn't need it at all, logic can be done in Stimulus

yeraydiazdiaz · 2018-03-30T17:05:41Z

warehouse/templates/index.html

+  {{ project_snippet(release, summary, index)}}
+</div>
+{%- endmacro %}
+{% macro project_snippet_trending(release, summary, index) -%}


these two macros differ only on the class, it might be cleaner to move the class to be an extra argument on the project_snippet macro below and just use that one?

Just noticed this, fixed it

yeraydiazdiaz · 2018-03-30T17:08:44Z

warehouse/static/js/warehouse/controllers/increment_controller.js

+  static targets = ["button"];
+
+  // Initially set default to 5 projects visible
+  constructor() {


rather than constructor Stimulus encourages other methods, in this case I'd opt for connect

yeraydiazdiaz · 2018-03-30T17:15:24Z

warehouse/static/js/warehouse/controllers/increment_controller.js

+  constructor() {
+    super();
+    this.trending_visible = 5;
+    this.latest_visible = 5;


I think it might be neater and simpler if instead of having the controller wrapping both columns, having one instance of a controller per column, i.e. the <div class="col-half">.

That would make remove all the complexity below on parsing and retrieving the eventType and the separate _visible variables.

You're right, it was hard to read

yeraydiazdiaz · 2018-03-30T17:32:47Z

warehouse/templates/index.html

 </div>
 {%- endmacro %}

+{% macro project_snippet(release, summary, index) -%}


do you need summary as an argument? seems you're retrieving it from release or defaulting anyway

Nope, unnecessary, you are right

jmuzsik · 2018-03-31T19:05:15Z

Thanks @yeraydiazdiaz! Believe that I have everything as you requested it to be. Definitely appreciated all the advice, definitely improved my ability with Stimulus and Jinga.

Let me know if you'd like anything else to change

yeraydiazdiaz

Hey, I left you some more comments, I probably should've explained what I meant better, Stimulus works a bit different than other frameworks I'm also getting used to it as well.

yeraydiazdiaz · 2018-04-01T19:13:43Z

warehouse/static/js/warehouse/controllers/show_trending_projects_controller.js

@@ -0,0 +1,42 @@
+/**


You don't need two controllers with the same code, Stimulus makes separate instances of the same class.

yeraydiazdiaz · 2018-04-01T19:15:38Z

warehouse/templates/index.html

+{% if class == "latest" %}
+<div class="package-snippet" data-target="show-latest-releases.latestRelease">
+{% else %}
+<div class="package-snippet" data-target="show-trending-projects.trendingProject">


if you use a single controller, you can avoid this if completely and use something like data-target="show-more.release"

I don't see how I can do this, show-more.latestReleases and show-more.trendingProjects are entirely different arrays, perhaps there is something clever I can do I don't think there is an explicit way I can do it

yeraydiazdiaz · 2018-04-01T19:16:41Z

warehouse/templates/index.html

-  <div class="site-container">
-    <div class="col-half">
+  <div class="site-container" >
+    <div class="col-half" data-controller="show-trending-projects">


you could simply use data-controller="show-more" here and below, Stimulus will create separate instances

yeraydiazdiaz · 2018-04-01T19:17:27Z

warehouse/templates/index.html

-      {#
-      <a class="button" href="TODO">Show More</a>
-      #}
+      <button class="button" type="button" data-action="click->show-trending-projects#addFive" data-target="show-trending-projects.addFiveButton">Show More</button>


remember to change the names on these, there won't be clashing as they're separate DOM elements and instances

yeraydiazdiaz · 2018-04-01T19:18:51Z

warehouse/templates/index.html

      <div class="heading-wsubtitle">
        <h2 class="heading-wsubtitle__heading">New Releases</h2>
        <p class="heading-wsubtitle__subtitle">Hot off the press: the newest project releases</p>
      </div>
      <div class="package-list">
        {% for release in latest_releases %}
-          {{ project_snippet(release) }}
+          {{ project_snippet(release, "latest") }}


I think you can even remove the class parameter?

not sure if that is possible, as I mentioned above

yeraydiazdiaz · 2018-04-01T19:27:05Z

warehouse/static/js/warehouse/controllers/show-latest-releases-controller.js

+  connect() {
+    this.amountVisible = 5;
+    let i = 0;
+    for (let element of this.latestReleaseTargets) {


it might be more compact to use some Array constructs:

this.releaseTargets.slice(this.amountVisible).forEach(element => { element.style.display = "none"; });

yeraydiazdiaz · 2018-04-03T11:08:12Z

Sorry, what I meant was something like e7befb8

The key is adding two data-controller instances, one per column, instead of a general one for both.

It might sound odd because we're using the same names for the targets but they're local to each data-controller so no clashing occurs. The benefit is that the controller is much simpler and will work on any number of columns.

jmuzsik · 2018-04-04T18:58:54Z

You are definitely right, I did not know that I could do this, but you basically wrote the entire PR based upon that code you wrote there. So perhaps you ought to send the PR? I mean if you want to I can close this and you can send it instead.

Either way, I'll put the working copy up. Thanks so much @yeraydiazdiaz!

yeraydiazdiaz

now that's just silly 😄 nice work on this 👍

yeraydiazdiaz · 2018-04-13T12:48:03Z

@jmuzsik up for resolving the conflicts and bringing this one home? 😄

jmuzsik · 2018-04-14T22:41:06Z

I didn't realise there were any! Thanks!

nlhkabu · 2018-04-17T17:43:10Z

hi @jmuzsik thanks for this PR and thanks also to @yeraydiazdiaz for your reviews :)
Just to let you know - this is on my list of things to pull down and review this week - sorry for the delay and thanks for your patience!

pradyunsg

I find it a little awkward that elements get added to the section, in place.

I think it would be more intuitive (to me) to be redirected to a search that shows results in the same order?

di · 2018-04-17T19:01:04Z

I think generally I'm in agreement with @pradyunsg. This also depends heavily on JS and won't work at all for users without JS or not in a modern browser, whereas linking to an ordered search result page would. I'll let have @nlhkabu have the final say here though.

nlhkabu · 2018-04-23T05:33:19Z

I agree with @pradyunsg and @di on this. If we were instead to direct the user to an ordered search results page, they could also apply search filters, which would give them more options. They would also benefit from any improvements we make to the search UI in the future.

@jmuzsik Is this something that you'd like to take on?

I have updated #1303 and #3447 accordingly.

jmuzsik · 2018-04-23T14:10:00Z

It makes complete sense to do it as you guys have spoken of, though I cannot do it right now, I would say in a few weeks I can begin to but I really am not sure. But if anyone else would like to take it on, by all means, maybe @yeraydiazdiaz. Thanks!

nlhkabu · 2018-04-25T19:41:18Z

ok great @jmuzsik :) Are you happy to close this PR in the meantime?

brainwane · 2018-04-30T15:39:20Z

@jmuzsik Thank you for your work on this. Please do let us know if you want thoughts on stuff to take on when you're more ready, in a few weeks!

jmuzsik added 3 commits March 26, 2018 02:50

first commit - show more when button is clicked on homepage

a1b5041

refactor - cleaner and got rid of unnecessary code

8c508d9

fix linting errors

0bb0e5c

drunkwcodes approved these changes Mar 27, 2018

View reviewed changes

brainwane mentioned this pull request Mar 27, 2018

add "search by new release" and "trending" sort orders, link to them from homepage #3447

Closed

yeraydiazdiaz suggested changes Mar 30, 2018

View reviewed changes

jmuzsik added 2 commits March 31, 2018 14:48

fix based upon requests

b78f84c

fix based upon requests

541ee87

yeraydiazdiaz suggested changes Apr 1, 2018

View reviewed changes

fix based upon requests

361346f

di self-assigned this Apr 2, 2018

Add separate instance of controller to each column

2a34133

yeraydiazdiaz approved these changes Apr 4, 2018

View reviewed changes

Merge branch 'master' into show_more

26ec41a

nlhkabu self-assigned this Apr 17, 2018

pradyunsg reviewed Apr 17, 2018

View reviewed changes

jmuzsik closed this Apr 30, 2018

Show more button on home page implementation #3424

Show more button on home page implementation #3424

Uh oh!

Conversation

jmuzsik commented Mar 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

drunkwcodes left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brainwane commented Mar 30, 2018

Uh oh!

yeraydiazdiaz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmuzsik commented Mar 31, 2018

Uh oh!

yeraydiazdiaz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yeraydiazdiaz commented Apr 3, 2018

Uh oh!

jmuzsik commented Apr 4, 2018

Uh oh!

yeraydiazdiaz left a comment

Choose a reason for hiding this comment

Uh oh!

yeraydiazdiaz commented Apr 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

jmuzsik commented Mar 26, 2018 •

edited

Loading

drunkwcodes left a comment •

edited

Loading

yeraydiazdiaz commented Apr 13, 2018 •

edited

Loading