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

preview of organization logo design on settings #4347

Merged
merged 1 commit into from
Oct 14, 2019
Merged

preview of organization logo design on settings #4347

merged 1 commit into from
Oct 14, 2019

Conversation

artoodeeto
Copy link
Contributor

  • created a shared partial logo_design file
  • moved text, bg and logo from profile.html.erb to logo_design.html.erb
  • used logo_design to org_admin

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Documentation Update

Description

org admin can preview dev.to design logo

Related Tickets & Documents

feature-request #4321. See also #4212

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

SAMPLE
ezgif com-optimize
DB
Screen Shot 2019-10-09 at 9 57 24 PM

mobile
Screen Shot 2019-10-09 at 8 49 23 PM

tablet
Screen Shot 2019-10-09 at 8 49 37 PM

desktop
Screen Shot 2019-10-09 at 8 50 04 PM

Added to documentation?

  • docs.dev.to
  • readme
  • no documentation needed

[optional] What gif best describes this PR or how it makes you feel?

shy

@artoodeeto
- created a shared partial logo_design file
d19f417 
- moved text, bg and logo from profile.html.erb to logo_design.html.erb
- used logo_design to org_admin
@pr-triage pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label Oct 10, 2019
@artoodeeto
Copy link
Contributor Author

hallooo there are some test that failed but unrelated to this issue.

 1) User visits articles by tag when user hasn't logged in when 2 articles shows the header
     Failure/Error: raise Pundit::NotAuthorizedError, "You must be logged in" unless user
     
     Pundit::NotAuthorizedError:
       You must be logged in

2) Reading list without tags when large readinglist shows the large reading list
     Failure/Error: params = { filters: "viewable_by:#{current_user.id}" }
     
     NoMethodError:
       undefined method `id' for nil:NilClass

rhymes
rhymes approved these changes Oct 10, 2019
View reviewed changes
Copy link
Contributor

@rhymes rhymes left a comment

Choose a reason for hiding this comment

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

Hi @artoodeeto, this looks good, also thanks for creating the shared partial!

I'm going to accept this but I'm going to request a review from other team members to make sure the UI is good enough, not just to me! There shouldn't be any problems since it's based on something that's already been deployed!

Great job!

@pr-triage pr-triage bot added PR: reviewed-approved bot applied label for PR's where reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Oct 10, 2019
@artoodeeto
Copy link
Contributor Author

@rhymes yea for sure. no problem. thank you. I didn't much do anything really. I just moved it to a different file. 😄

@coolbrg
Copy link

coolbrg commented Oct 10, 2019

hallooo there are some test that failed but unrelated to this issue.

Just noticed I am also able to reproduce the 2nd error. See #4349.

@Link2Twenty
Copy link
Contributor

Now that there is a partial we could in easily add a contrast indicator that will exist on each instance 🎉 🎉

coolbrg
coolbrg suggested changes Oct 10, 2019
View reviewed changes
Copy link

@coolbrg coolbrg left a comment

Choose a reason for hiding this comment

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

Hi @artoodeeto ,

You forgot to update one file. Please check coolbrg@3c74ffc

PR looks nice now. 👍

@pr-triage pr-triage bot added PR: reviewed-changes-requested bot applied label for PR's where reviewer requests changes and removed PR: reviewed-approved bot applied label for PR's where reviewer approves changes labels Oct 10, 2019
@artoodeeto
Copy link
Contributor Author

@coolbrg opps. thank you. thought only for admins. 😄

jessleenyc
jessleenyc approved these changes Oct 11, 2019
View reviewed changes
Copy link
Contributor

@jessleenyc jessleenyc left a comment

Choose a reason for hiding this comment

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

lgtm!

@jessleenyc jessleenyc added PR: reviewed-approved bot applied label for PR's where reviewer approves changes and removed PR: reviewed-changes-requested bot applied label for PR's where reviewer requests changes labels Oct 11, 2019
@weekly-digest weekly-digest bot mentioned this pull request Oct 14, 2019
Closed
@benhalpern benhalpern merged commit 296ebeb into forem:master Oct 14, 2019
@pr-triage pr-triage bot added PR: merged bot applied label for PR's that are merged and removed PR: reviewed-approved bot applied label for PR's where reviewer approves changes labels Oct 14, 2019
@artoodeeto artoodeeto mentioned this pull request Oct 14, 2019
Closed
RakChamp25 pushed a commit to RakChamp25/dev.to that referenced this pull request Oct 15, 2019
@artoodeeto @RakChamp25
- created a shared partial logo_design file (forem#4347)
72c27aa 
- moved text, bg and logo from profile.html.erb to logo_design.html.erb
- used logo_design to org_admin
@artoodeeto artoodeeto mentioned this pull request Oct 16, 2019
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@coolbrg coolbrg coolbrg requested changes

@jessleenyc jessleenyc jessleenyc approved these changes

@rhymes rhymes rhymes approved these changes

@benhalpern benhalpern Awaiting requested review from benhalpern

Assignees
No one assigned
Labels
PR: merged bot applied label for PR's that are merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@artoodeeto @coolbrg @Link2Twenty @rhymes @jessleenyc @benhalpern