Update home and remove menu #337

rafamanzo · 2016-04-13T20:01:48Z

Information got update regarding PHP and copyright year.

The footer became a grid in order to better work on smaller screens.

This will isolate upcoming changes on the menu items. Signed-off-by: Rafael Reggiani Manzo <[email protected]>

This will make possible to remove the left side box which now has no purpose. Signed-off-by: Heitor Reis <[email protected]>

Its user related features have been moved to the header and tutorials are now under mezuro.github.io. The remaining Kalibro informations were just placeholders and are now better covered on mezuro.github.io as well. Signed-off-by: Heitor Reis <[email protected]>

The 'Hello' just felt wrong in the top menu (design stuff). Signed-off-by: Heitor Reis <[email protected]>

It will enable better isolation when turning it into a Bootstrap container-fluid just like the header and body. Signed-off-by: Heitor Reis <[email protected]>

It now has to click first on the dorpdown.

This improves the behaviour on small screen devices.

This is a information that will get published at the homepage.

The acceptance test has been refactored accordingly and got more extended to untested features.

diegoamc · 2016-04-13T20:57:40Z

👍 Nice work!
@mezuro/core . Please review this one too.

do-you-dare · 2016-04-13T23:28:12Z

Great job! 😄

danielkza · 2016-04-13T23:38:15Z

app/models/repository.rb

@@ -1,3 +1,7 @@
 class Repository < KalibroClient::Entities::Processor::Repository
  include KalibroRecord
+
+  def self.latest(count=1)
+    all.sort { |one, another| another.id <=> one.id }.first(count)


This will fetch every single repository any time it's called. The sort should be changed to something that ActiveRecord can handle to avoid that.

This is not activerecord, but KalibroClient... The all will retrieve an array once per call and the sort will produce no other calls as it is implemented by array.

Do you have a better implementation?

That's what I get for writing when I'm sleepy. I misread it completely, sorry.

But I remembered just now that we can use RepositoryAttributes to up repositories while actually using ActiveRecord.

And then call KalibroClient? Involving RepositoriesAttributes (which are not called currently) does not add more complexity? And more complexity on understanding what the code does? The cache is not supposed to make such queries affordable?

And then call KalibroClient? Involving RepositoriesAttributes (which are not called currently) does not add more complexity? And more complexity on understanding what the code does? The cache is not supposed to make such queries affordable?

The added complexity is just .map(&:repository). It surely beats fetching all repositories over the network to manually sort them. Even if we cache it, we're turning a query that should take miliseconds in one that can possibly take seconds while using up a ridiculous amount of database I/O for no reason.

What I wrote was meant to address all of the questions, which ones in particular do you mean? I don't think the cache justifies writing code that is two or three orders of magnitude less optimal than what is possible (and without that much effort).

Maybe a third opinion would be nice @mezuro/core

I just want to stress again: code readability is more important than performance, if you can address the perfomance issues by other techniques such as caching, just have a look through all the other projects. Any change should be subject of a retrospective rather than issue comments.

There will be no changes regarding this on this PR.

As you wish.

Maybe we can measure the time it takes on both cases. If the difference is really of two or three orders of magnitude, I think we should do it even if readability gets worse.

The reason for that is we do not want that our pages take more than a few hundreds of milliseconds to load. Specially our home page, even on a few cases (when cache needs to be updated). There's been a lot of research concluding that users will not return to your site if the pages take too long to load (usually that means at most 1 second).

It seems to be way too much effort just for this. I'm fine leaving it as
is, even though I think it's not nearly the best code we could write
considering the balance between readability and efficiency. But I think
it's important to understand well how the cache will work before using it
as a replacement for even thinking about performance. As usual, "no
silver bullet" is relevant here.

Also, we're introducing the recent things listings as an UX improvement.
How can we know if it will be useful if we don't even know how often it
will be updated?

Em qui, 14 de abr de 2016 12:33, Diego de Araújo Martinez Camarinha <
[email protected]> escreveu:

In app/models/repository.rb
#337 (comment):

@@ -1,3 +1,7 @@
class Repository < KalibroClient::Entities::Processor::Repository
include KalibroRecord
+

def self.latest(count=1)

all.sort { |one, another| another.id <=> one.id }.first(count)

Maybe we can measure the time it takes on both cases. If the difference is
really of two or three orders of magnitude, I think we should do it even if
readability gets worse.

The reason for that is we do not want that our pages take more than a few
hundreds of milliseconds to load. Specially our home page, even on a few
cases (when cache needs to be updated). There's been a lot of research
concluding that users will not return to your site if the pages take too
long to load (usually that means at most 1 second).

—
You are receiving this because you are on a team that was mentioned.

Reply to this email directly or view it on GitHub
https://github.com/mezuro/prezento/pull/337/files/610763c4f6a22b9db9bcf044a0fa990a50ded422#r59739014

danielkza · 2016-04-14T01:16:24Z

I forgot to mention, but good job, my comments are just nitpicks :)

danielkza · 2016-04-14T13:52:41Z

app/controllers/home_controller.rb

@@ -1,5 +1,7 @@
 class HomeController < ApplicationController
  def index
    @latest_projects = Project.latest(5)
+    @latest_repositories = Repository.latest(5)
+    @latest_configurations = KalibroConfiguration.latest(5)


I just noticed it - doesn't this defeat the cache completely? Since it's not inside a cache block it will always be executed before the view is rendered, and always fetch everything.

Nice catch (#339) as this seems as a bug introduced back in 2013 and just found now.

As we are using multiple cache fragments for one action, we need to specify action_suffixes so rails create different keys for each fragment. Signed-off-by: Rafael Reggiani Manzo <[email protected]>

marcheing and others added 12 commits April 13, 2016 14:24

Refactor navbar to a partial on application view

6de27be

This will isolate upcoming changes on the menu items. Signed-off-by: Rafael Reggiani Manzo <[email protected]>

Move user links to header dropdown

22bba08

This will make possible to remove the left side box which now has no purpose. Signed-off-by: Heitor Reis <[email protected]>

Remove left side menu

e6328e3

Its user related features have been moved to the header and tutorials are now under mezuro.github.io. The remaining Kalibro informations were just placeholders and are now better covered on mezuro.github.io as well. Signed-off-by: Heitor Reis <[email protected]>

Remove unecessary greeting on header

49c7765

The 'Hello' just felt wrong in the top menu (design stuff). Signed-off-by: Heitor Reis <[email protected]>

Extract footer into a partial

826a08f

It will enable better isolation when turning it into a Bootstrap container-fluid just like the header and body. Signed-off-by: Heitor Reis <[email protected]>

Fix user edition acceptance test

701166d

It now has to click first on the dorpdown.

Turn footer into a grid

3bf7a59

This improves the behaviour on small screen devices.

Update home text to reflect PHP support

d0369ac

Add latest KalibroConfigurations listing

50c4dc6

This is a information that will get published at the homepage.

Create latest Repositories listing method

a9dd14e

This is a information that will get published at the homepage.

Add latest Repositories and Configurations to home

610763c

The acceptance test has been refactored accordingly and got more extended to untested features.

danielkza reviewed Apr 13, 2016
View reviewed changes

danielkza reviewed Apr 14, 2016
View reviewed changes

Fix cache creation on homepage

f0955ba

As we are using multiple cache fragments for one action, we need to specify action_suffixes so rails create different keys for each fragment. Signed-off-by: Rafael Reggiani Manzo <[email protected]>

diegoamc merged commit 0b32880 into master Apr 15, 2016

diegoamc deleted the update_home_again branch April 15, 2016 18:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update home and remove menu #337

Update home and remove menu #337

rafamanzo commented Apr 13, 2016

diegoamc commented Apr 13, 2016

do-you-dare commented Apr 13, 2016

danielkza Apr 13, 2016

rafamanzo Apr 14, 2016

danielkza Apr 14, 2016

rafamanzo Apr 14, 2016

danielkza Apr 14, 2016

danielkza Apr 14, 2016

rafamanzo Apr 14, 2016

danielkza Apr 14, 2016

diegoamc Apr 14, 2016

danielkza Apr 14, 2016

danielkza commented Apr 14, 2016

danielkza Apr 14, 2016

rafamanzo Apr 14, 2016

Update home and remove menu #337

Update home and remove menu #337

Conversation

rafamanzo commented Apr 13, 2016

diegoamc commented Apr 13, 2016

do-you-dare commented Apr 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielkza commented Apr 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment