Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Adding homepage header and cards seeds #2679

Merged
merged 1 commit into from
Jun 22, 2018
Merged

Adding homepage header and cards seeds #2679

merged 1 commit into from
Jun 22, 2018

Conversation

PierreMesure
Copy link
Collaborator

References

Objectives

  • Add dev seeds to customize the homepage

Visual Changes

Before:

screenshot-2018-6-13 consul 1

After:

screenshot-2018-6-13 consul

Notes

  • I added images in the dev_seeds folder, not sure if that is good practice.
  • Also not sure about the way I'm creating the image attachments for the widgets. It works but is it the best way?

@PierreMesure
Copy link
Collaborator Author

Also these comments:

  • the title and the label of a card don't appear if there's no picture
  • the text of the 'Save card' button changes to <span class=&quot;translation_missing&quot; title=&quot;translation missing: en.admin.homepage.update.submit_card&quot;>Submit Card</span>when there is an error in the card form
  • in the card form, the image is displayed cropped and enlarged for no reason
  • (design) the card description is underlined on hover, which makes it less readable. Wouldn't it be better to design a hover state for the whole card if it is meant to be clickable?

aitbw
aitbw reviewed Jun 13, 2018
View reviewed changes
Gemfile.lock Outdated
@@ -41,7 +41,7 @@ GEM
acts_as_votable (0.11.1)
addressable (2.5.2)
public_suffix (>= 2.0.2, < 4.0)
ahoy_matey (1.6.0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did you update this file? I know most of them all minor updates but this PR should be focused on only adding seeds to populate the homepage

db/dev_seeds/widgets.rb
header: FALSE,
image_attributes: create_image_attachment('budget')
)
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please add a trailing whitespace at the end of this file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

lib/tasks/homepage.rake Outdated
@@ -10,5 +10,4 @@ namespace :homepage do
Setting['feature.homepage.widgets.feeds.processes'] = true
end
end

end
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please add a trailing whitespace at the end of this file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

aitbw
aitbw reviewed Jun 13, 2018
View reviewed changes
db/dev_seeds/widgets.rb Outdated

def create_image_attachment(type)
{
cached_attachment: File.new("db/dev_seeds/images/#{type}_background.jpg"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please, use Rails.root.join instead of File.new

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

aitbw
aitbw reviewed Jun 13, 2018
View reviewed changes
db/dev_seeds/widgets.rb Outdated
{
cached_attachment: File.new("db/dev_seeds/images/#{type}_background.jpg"),
title: "#{type}_background.jpg",
user: User.where(id: 1).first
Copy link
Collaborator

Choose a reason for hiding this comment

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

You don't need to specify a .where here, User.first is more than enough 😌

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@aitbw
Copy link
Collaborator

aitbw commented Jun 13, 2018

Hi Pierre! 👋 Thanks for collaborating once again with CONSUL!

Please, take your time to review my comments. Feel free to ask anything before updating your PR ☺️

@PierreMesure
Copy link
Collaborator Author

Thanks, @aitbw. I applied the changes you recommended.

Can you confirm that db/dev_seeds/images is a good location for images?

@decabeza, I chose images from Unsplash but they are quite random. Feel free to give feedback about that.

@voodoorai2000 and @decabeza, since you were the one to implement #2641, what do you think about the points I mentioned up there?

@aitbw
Copy link
Collaborator

aitbw commented Jun 14, 2018

Great @PierreMesure ! 🎉

To complete my review, could you please drop commits 9d88e5f and f698471 and fixup commit 9cf089a? This is to exclude the Gemfile.lock-related commits from this PR and to have just 1 commit for this proposed change —this is because:

  1. To keep Git history as clean as possible
  2. Such a small change does not require several commits in order to complete an issue

Let me know if you need anything else ☺️

@voodoorai2000
Copy link
Member

@PierreMesure this is great! Thank you so much 😌
Sure, db/dev_seeds/images is good 👍
Thanks for noting the other polishing details 😌we'll create new issues to fix them ;)

@PierreMesure PierreMesure force-pushed the 2667-Create-dev-seeds-to-homepage branch 2 times, most recently from 66ff7a4 to 8f11f21 Compare June 15, 2018 12:17
@PierreMesure PierreMesure force-pushed the 2667-Create-dev-seeds-to-homepage branch from 676082c to 1ebcf04 Compare June 15, 2018 12:27
@PierreMesure
Copy link
Collaborator Author

@aitbw Thanks for your review. I reset the commits instead of squashing them because I was struggling to force push it. I agree it looks cleaner but I'm not sure it's a good practice for bigger changes where the history of a branch can be useful.

@aitbw
Copy link
Collaborator

aitbw commented Jun 15, 2018

Thanks @PierreMesure ! 🎉

And yes, you are correct: for bigger PRs it is not recommended doing that but for this one, it's better to keep it as clean and concise as possible 😌

@PierreMesure
Copy link
Collaborator Author

I messed with the CI by deleting some commits but the last build succeeded on my fork.

@PierreMesure
Copy link
Collaborator Author

PierreMesure commented Jun 20, 2018 •
edited

@aitbw, @voodoorai2000 Is there anything more I need to do before it gets merged?
Also, should I create the issues I noticed or let you do it?

@decabeza
Copy link
Collaborator

Hi @PierreMesure! sorry for the delay on the response 🙏

Don't worry about random images, are ok! 😉

I created a new issue with all your comments!

Many thanks! 😌

@decabeza decabeza changed the title Adding seeds Adding homepage header and cards seeds Jun 21, 2018
@decabeza decabeza merged commit 6133afa into consuldemocracy:master Jun 22, 2018
This was referenced Jun 27, 2018
Closed
Closed
@decabeza decabeza mentioned this pull request Jun 29, 2018
Merged
@voodoorai2000 voodoorai2000 mentioned this pull request Jul 20, 2018
Closed
@clairezed clairezed mentioned this pull request Aug 4, 2018
Merged
@decabeza decabeza added this to Release 0.17 in Roadmap Oct 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aitbw aitbw aitbw left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@PierreMesure @aitbw @voodoorai2000 @decabeza