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

UI: New share button and toolbar reorganize #77563

Merged
merged 11 commits into from
Jan 8, 2024

Conversation

evictorero
Copy link
Contributor

@evictorero evictorero commented Nov 2, 2023

What is this feature?
Changes style of the public dashboard badge, the add and share button and reorganize toolbar buttons.

Before
image

Now
image
image

Which issue(s) does this PR fix?:

Fixes #77298
Fixes #71539

Special notes for your reviewer:
This is the first step of toolbar redesign, more info here

Please check that:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

@evictorero evictorero requested a review from a team as a code owner November 2, 2023 13:12
@evictorero evictorero requested review from dprokop and kaydelaney and removed request for a team November 2, 2023 13:12
@grafana-delivery-bot grafana-delivery-bot bot added this to the 10.3.x milestone Nov 2, 2023
@evictorero evictorero requested a review from a team as a code owner November 2, 2023 14:19
@evictorero evictorero requested review from Clarity-89 and ashharrison90 and removed request for a team November 2, 2023 14:19
@natydej
Copy link

natydej commented Nov 8, 2023

We decided to enable a feature flag to control and test this version V1, and get some feedback from users to see how it will impact the usage.

V1

Figma

cc @thanos-karachalios

@evictorero
Copy link
Contributor Author

Blocked until we implement tracking in the toolbar actions.

Copy link
Contributor

@lucychen-grafana lucychen-grafana left a comment

Choose a reason for hiding this comment

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

Tested and LGTM

@natellium
Copy link
Contributor

@evictorero Something I just thought is that we should make sure no docs need to be updated (or that they get updated if needed), we may be using some screenshots that need to be renewed.

@evictorero
Copy link
Contributor Author

@evictorero Something I just thought is that we should make sure no docs need to be updated (or that they get updated if needed), we may be using some screenshots that need to be renewed.

We will wait a few days to collect analytics data from the old button before merge this one. I don't think we update screenshots in our docs for these small changes do we? In any case, this will change a lot over the next weeks/months maybe we can wait until a more refined version?

@natellium
Copy link
Contributor

We will wait a few days to collect analytics data from the old button before merge this one. I don't think we update screenshots in our docs for these small changes do we? In any case, this will change a lot over the next weeks/months maybe we can wait until a more refined version?

I actually don't know :) @imatwawana any thoughts?

@imatwawana
Copy link
Collaborator

imatwawana commented Dec 4, 2023

We will wait a few days to collect analytics data from the old button before merge this one. I don't think we update screenshots in our docs for these small changes do we? In any case, this will change a lot over the next weeks/months maybe we can wait until a more refined version?

I actually don't know :) @imatwawana any thoughts?

You can probably hold off on updating screenshots if there are going to be several changes, but the text should be updated as elements get changed. So the docs where we refer to the "sharing icon" should be updated along with this change.

@grafana-pr-automation grafana-pr-automation bot requested review from a team and oshirohugo and removed request for a team January 4, 2024 19:14
Copy link
Contributor

@juanicabanas juanicabanas left a comment

Choose a reason for hiding this comment

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

LGTM!
As we discussed, I'm a little worried about the button padding and height, which is not the same as in the design.
I agree we shouldn't customize the CSS as it was before, though.

Copy link
Contributor

@ivanortegaalba ivanortegaalba left a comment

Choose a reason for hiding this comment

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

Looks great to me! 💯

Please wait for the review from @Ijin08 if he is not aware of the redesign 🙏

2024-01-05 11 20 54

@@ -332,7 +309,33 @@ export const DashNav = React.memo<Props>((props) => {
);
}

addCustomContent(dynamicDashNavActions.right, buttons);
if (canEdit && !isFullscreen) {
if (config.featureToggles.emptyDashboardPage) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove this condition since the toggle is enabled to 100% by default 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I searched for this flag and it is being used in more places, I prefer to leave it here to be removed by a cleanup PR.

@@ -360,3 +363,9 @@ const modalStyles = css({
width: 'max-content',
maxWidth: '80vw',
});

const publicBadgeStyle = css({
color: 'grey',
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't we use grey as color, or event transparent? Blue is confusing 😓

Copy link
Contributor Author

@evictorero evictorero Jan 5, 2024

Choose a reason for hiding this comment

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

We use grey on this line 🤔. If you are talking about the original <Badge line it is using blue because it is one of the valid colors required by BadgeComponent https://github.com/grafana/grafana/blob/329ec2624a514afa0c042800a00341fdc811e068/packages/grafana-ui/src/components/Badge/Badge.tsx#L14```

I don't like it, but I don't know how to fix it and it should be temporary until we replace it with the new Badge component.

Copy link

@natydej natydej left a comment

Choose a reason for hiding this comment

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

LGTM!

@evictorero evictorero merged commit 25ff4ba into main Jan 8, 2024
20 checks passed
@evictorero evictorero deleted the evictorero/new-toolbar-design branch January 8, 2024 13:42
@ifrost ifrost modified the milestones: 10.3.x, 10.4.x Jan 12, 2024
s0lesurviv0r pushed a commit to s0lesurviv0r/grafana that referenced this pull request Feb 3, 2024
@aangelisc aangelisc modified the milestones: 10.4.x, 10.4.0 Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Task: Implement new design for the Share button Improve the "Public" tag functionality
9 participants