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

[MM-58405] Migrate tooltips of 'components/announcement_bar/default_announcement_bar/announcement_bar' to WithTooltip #27244

Merged
merged 9 commits into from
Jul 1, 2024
Merged

[MM-58405] Migrate tooltips of 'components/announcement_bar/default_announcement_bar/announcement_bar' to WithTooltip #27244

merged 9 commits into from
Jul 1, 2024

Conversation

MeHow25
Copy link
Contributor

@MeHow25 MeHow25 commented Jun 2, 2024

Summary

Migrate tooltips of 'components/announcement_bar/default_announcement_bar/announcement_bar' to WithTooltip.

Ticket Link

Fixes #27158
Jira https://mattermost.atlassian.net/browse/MM-58405

Screenshots

component before after
CloudTrialAnnouncementBar image image
CloudTrialEndAnnouncementBar image image
ConfigurationAnnouncementBar image image
NotificationPermissionBar image image
OverageUsersBanner image image
PaymentAnnouncementBar image image
TextDismissableBar image image
ToPaidPlanBannerDismissable image image
UsersLimitsAnnouncementBar image image
VersionBar image image

Release Note

Made the appearance of several tooltips more consistent

@mm-cloud-bot mm-cloud-bot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jun 2, 2024
@mattermost-build
Copy link
Contributor

Hello @MeHow25,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

larkox
larkox reviewed Jun 10, 2024
View reviewed changes
Copy link
Contributor

@larkox larkox left a comment

Choose a reason for hiding this comment

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

A couple of comments on this.

webapp/channels/src/components/with_tooltip/index.tsx Outdated Show resolved Hide resolved
webapp/channels/src/components/with_tooltip/index.tsx Show resolved Hide resolved
webapp/channels/src/components/announcement_bar/default_announcement_bar/announcement_bar.tsx Outdated
@@ -170,11 +164,12 @@ export default class AnnouncementBar extends React.PureComponent<Props, State> {
css={{gridArea: 'announcement'}}
data-testid={this.props.id}
>
<OverlayTrigger
delayShow={Constants.OVERLAY_TIME_DELAY}
<WithTooltip
Copy link
Contributor

Choose a reason for hiding this comment

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

@M-ZubairAhmed Out of the scope of this PR, and probably something we won't tackle for a while, but I feel the announcement banner is making a weird use of the tooltips. Maybe we should revisit the announcement banner as a whole with UX to clarify how it should look and behave, instead of being so "free" as it is now.

Copy link
Member

Choose a reason for hiding this comment

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

@larkox i definetly agree with that do you think we can merge this pr without those change or should we fix that and then come back to this one?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would say let's merge this one, since it is practically done, and we can open a more detailed effort afterwards.

Copy link
Member

Choose a reason for hiding this comment

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

Feel free to open a ticket with proposed fix when you get time

@M-ZubairAhmed M-ZubairAhmed added 2: Dev Review Requires review by a developer 3: QA Review Requires review by a QA tester. May occur at the same time as Dev Review labels Jun 20, 2024
@unified-ci-app
Copy link
Contributor

Not triggering E2E tests: this PR has 8 commit status(es) or check-runs that are not passing. Ensure all statuses aside from the E2E testing ones are green, before triggering E2E tests.

@M-ZubairAhmed
Copy link
Member

/update-branch

M-ZubairAhmed
M-ZubairAhmed approved these changes Jun 25, 2024
View reviewed changes
Copy link
Member

@M-ZubairAhmed M-ZubairAhmed left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for working through this :), can you please check the failing test ci @MeHow25

@MeHow25 MeHow25 requested a review from a team as a code owner June 26, 2024 17:30
@MeHow25
Copy link
Contributor Author

MeHow25 commented Jun 26, 2024

Thanks for working through this :), can you please check the failing test ci @MeHow25

Sorry. I forgot to remove some unnecessary code. I removed it now and it should be ok.

@agnivade agnivade removed the request for review from a team June 27, 2024 04:39
@M-ZubairAhmed
Copy link
Member

/update-branch

@M-ZubairAhmed M-ZubairAhmed removed the 2: Dev Review Requires review by a developer label Jun 27, 2024
@yasserfaraazkhan yasserfaraazkhan added the Setup Cloud Test Server Setup an on-prem test server label Jun 28, 2024
@larkox larkox added 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester. May occur at the same time as Dev Review labels Jul 1, 2024
@larkox larkox merged commit f36eb59 into mattermost:master Jul 1, 2024
15 checks passed
@mattermost-build mattermost-build removed the Setup Cloud Test Server Setup an on-prem test server label Jul 1, 2024
@mm-cloud-bot
Copy link

Test server destroyed

@amyblais amyblais added Changelog/Done Required changelog entry has been written Docs/Not Needed Does not require documentation labels Jul 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@larkox larkox larkox approved these changes

@yasserfaraazkhan yasserfaraazkhan yasserfaraazkhan approved these changes

@M-ZubairAhmed M-ZubairAhmed M-ZubairAhmed approved these changes

Assignees
No one assigned
Labels
4: Reviews Complete All reviewers have approved the pull request Changelog/Done Required changelog entry has been written Contributor Docs/Not Needed Does not require documentation release-note Denotes a PR that will be considered when it comes time to generate release notes.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Migrate tooltips of 'components/announcement_bar/default_announcement_bar/announcement_bar' to WithTooltip
7 participants
@MeHow25 @mattermost-build @M-ZubairAhmed @mm-cloud-bot @larkox @yasserfaraazkhan @amyblais