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.

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

NewsDrawer: Add grot to news drawer (after news items) #68173

Merged
merged 9 commits into from
May 12, 2023
Merged

Conversation

torkelo
Copy link
Member

@torkelo torkelo commented May 10, 2023

Ok final version a more subtle one hidden after the last news item:

Grafana.1.webm

Discarded ideas (placing him in the header before the drawer title)
image

A bit bigger
image

A lot bigger:
Screenshot from 2023-05-10 09-57-29

@torkelo torkelo requested a review from a team as a code owner May 10, 2023 07:58
@torkelo torkelo requested review from joshhunt and L-M-K-B and removed request for a team May 10, 2023 07:58
@torkelo torkelo added this to the 10.0.0 milestone May 10, 2023
@torkelo torkelo requested review from ergoerik, amy-super and staton-hyse11 and removed request for joshhunt and L-M-K-B May 10, 2023 08:01
@ergoerik
Copy link

@torkelo i think this doesn't quite feel right. Grot has to get too big for the meaning of the image to be clear. In this placement I think the larger it gets the more distracting and out of place it feels. We can try for a simpler version but I would probably skip.

@torkelo
Copy link
Member Author

torkelo commented May 10, 2023

@ergoerik what about this? Having it at the bottom for those that happen to scroll to the end, he becomes more subtle, almost like an easter egg

-api-ds-query---HTTP-handlers---Grafana-Monitoring---Scenes---Dashboards---Grafana.webm

@ergoerik
Copy link

@torkelo here's an updated image with little spiral on the newspaper

Newspaper Grot small spiral

@ergoerik
Copy link

@bogdanracles @jdcarolyne FYI

@torkelo torkelo changed the title NewsDrawer: Add grot to drawer header NewsDrawer: Add grot to news drawer (after news items) May 10, 2023
Copy link

@amy-super amy-super left a comment

Choose a reason for hiding this comment

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

I think it's cute to include Grot here for sure and love the updated image with our spiral on the newspaper. I just wonder if having it at the bottom means it will be invisible. This doesn't hurt anything so I'm game to include it, but I wonder if we can pull down how often news is being read.

@torkelo
Copy link
Member Author

torkelo commented May 10, 2023

@amy-super yea, I don't think it will be very common for people to scroll to the bottom, which this is more of a easter egg :)

But I can't think of another way to fit him in that doesn't feel out of place. Do you have any ideas?

@ergoerik
Copy link

What about something like this @torkelo @amy-super
Screenshot 2023-05-10 at 4 22 10 PM

@torkelo
Copy link
Member Author

torkelo commented May 11, 2023

@ergoerik the drawer opens under topnav so there is not that much room for a larger grot

Something like this could maybe work? @amy-super what do you think?
image

@torkelo
Copy link
Member Author

torkelo commented May 11, 2023

Or this
image

@ergoerik
Copy link

@ergoerik the drawer opens under topnav so there is not that much room for a larger grot

Something like this could maybe work? @amy-super what do you think? image

I think this could work. Prefer this to the option with Grot right aligned because it connects the words and image as a single idea/thought. If this small, Grot without the amoeba could work better:

Grot-news-3

@amy-super
Copy link

Agree I prefer the centered one - and dropping the amoeba should be OK in dark mode as well. And in the end, I think it's OK that Grot is a little smaller but more visible (over the idea of dropping it at the end).

@staton-hyse11
Copy link
Contributor

This looks awesome! I agree with Amy's comments on this - centered and amoeba free 😄

@torkelo torkelo requested a review from grafanabot as a code owner May 12, 2023 08:08
@torkelo
Copy link
Member Author

torkelo commented May 12, 2023

Ok pushed an update.

I think this size works pretty well, you clearly see what Grot is doing but it's not taking too much space from the drawer (news items).

Oh, and I added a border. With or without border that is the question?

@ergoerik @amy-super @staton-hyse11

image

Copy link
Contributor

@staton-hyse11 staton-hyse11 left a comment

Choose a reason for hiding this comment

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

LGTM

@amy-super
Copy link

With the border! Looks great ❤️

@torkelo torkelo added add to changelog product-approved Pull requests that are approved by product/managers and are allowed to be backported backport v10.0.x labels May 12, 2023
@torkelo torkelo enabled auto-merge (squash) May 12, 2023 14:38
@torkelo torkelo merged commit c8fd3c2 into main May 12, 2023
15 checks passed
@torkelo torkelo deleted the grot-reading-news branch May 12, 2023 14:38
@grafanabot
Copy link
Contributor

The backport to v10.0.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new branch
git switch --create backport-68173-to-v10.0.x origin/v10.0.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x c8fd3c20cd9f5a30ea3908f5da44ef5f92deb10d
# Push it to GitHub
git push --set-upstream origin backport-68173-to-v10.0.x
git switch main
# Remove the local backport branch
git branch -D backport-68173-to-v10.0.x

Then, create a pull request where the base branch is v10.0.x and the compare/head branch is backport-68173-to-v10.0.x.

@grafanabot grafanabot added the backport-failed Failed to generate backport PR. Please resolve conflicts and create one manually. label May 12, 2023
@torkelo torkelo added backport v10.0.x and removed backport v10.0.x backport-failed Failed to generate backport PR. Please resolve conflicts and create one manually. labels May 15, 2023
grafanabot pushed a commit that referenced this pull request May 15, 2023
* NewsDrawer: Add grot to drawer header

* Update

* Move to bottom

* Updates

* reverted unrelated change

* Update

* fixing test

(cherry picked from commit c8fd3c2)
torkelo added a commit that referenced this pull request May 15, 2023
)

NewsDrawer: Add grot to news drawer (after news items)  (#68173)

* NewsDrawer: Add grot to drawer header

* Update

* Move to bottom

* Updates

* reverted unrelated change

* Update

* fixing test

(cherry picked from commit c8fd3c2)

Co-authored-by: Torkel Ödegaard <torkel@grafana.com>
ryantxu pushed a commit that referenced this pull request May 16, 2023
* NewsDrawer: Add grot to drawer header

* Update

* Move to bottom

* Updates

* reverted unrelated change

* Update

* fixing test
@zerok zerok modified the milestones: 10.0.0, 10.0.0-preview, 10.1.x May 31, 2023
@ricky-undeadcoders ricky-undeadcoders modified the milestones: 10.1.x, 10.1.0 Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
add to changelog area/frontend backport v10.0.x product-approved Pull requests that are approved by product/managers and are allowed to be backported
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants