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

fix: adjust button tooltips to consistent style #2018

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

rackrick
Copy link
Member

@rackrick rackrick commented Sep 19, 2024

Description

This PR adjust the style of the tooltips from the HistoryListPanel.vue to be alle the same.
The Delete button had a different tooltip and the settings button had none.

Related Tickets & Documents

Mobile & Desktop Screenshots/Recordings

Before:
grafik

After:
grafik

[optional] Are there any post-deployment tasks we need to perform?

Summary by Sourcery

Adjust button tooltips in the HistoryListPanel.vue component to ensure a consistent style by adding missing tooltips and modifying existing ones.

Bug Fixes:

  • Ensure consistent tooltip style across buttons in the HistoryListPanel.vue component by adding missing tooltips and adjusting existing ones.

@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Sep 19, 2024
Copy link
Contributor

sourcery-ai bot commented Sep 19, 2024

Reviewer's Guide by Sourcery

This pull request adjusts the button tooltips in the HistoryListPanel.vue component to ensure a consistent style across all buttons. The changes include adding a missing tooltip for the settings button and standardizing the tooltip implementation for the delete button.

File-Level Changes

Change Details Files
Standardize tooltip implementation for the delete button
  • Wrap the delete button in a v-tooltip component
  • Move the title attribute to a separate span element within the tooltip
  • Implement v-bind and v-on directives for proper tooltip functionality
src/components/panels/HistoryListPanel.vue
Add tooltip for the settings button
  • Wrap the settings button in a v-tooltip component
  • Add a span element with the tooltip text
  • Implement v-bind and v-on directives for proper tooltip functionality
  • Use spread operator to combine tooltip and menu activation
src/components/panels/HistoryListPanel.vue
Add new translation key for settings tooltip
  • Add 'Settings' key under the 'History' section in the English locale file
src/locales/en.json

Tips
  • Trigger a new Sourcery review by commenting @sourcery-ai review on the pull request.
  • Continue your discussion with Sourcery by replying directly to review comments.
  • You can change your review settings at any time by accessing your dashboard:
    • Enable or disable the Sourcery-generated pull request summary or reviewer's guide;
    • Change the review language;
  • You can always contact us if you have any questions or feedback.

Copy link
Contributor

Language file analysis report:

File Missing Keys Unused Keys
en.json 0 0

sourcery-ai[bot]
sourcery-ai bot previously approved these changes Sep 19, 2024
Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @rackrick - I've reviewed your changes - here's some feedback:

Overall Comments:

  • There's a redundant tooltip text for the settings menu. Consider removing the duplicate element inside the v-menu component.
  • Consider adding or updating UI tests to cover these changes, if applicable.
Here's what I looked at during the review
  • 🟡 General issues: 3 issues found
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

src/components/panels/HistoryListPanel.vue Show resolved Hide resolved
src/components/panels/HistoryListPanel.vue Show resolved Hide resolved
src/components/panels/HistoryListPanel.vue Show resolved Hide resolved
@rackrick rackrick changed the title fix: adjust button tooltips to same style fix: adjust button tooltips to consistent style Sep 19, 2024
Copy link
Contributor

Language file analysis report:

File Missing Keys Unused Keys
en.json 0 0

@rackrick
Copy link
Member Author

@sourcery-ai review

sourcery-ai[bot]
sourcery-ai bot previously approved these changes Sep 19, 2024
Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @rackrick - I've reviewed your changes and they look great!

Here's what I looked at during the review
  • 🟡 General issues: 2 issues found
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

src/components/panels/HistoryListPanel.vue Show resolved Hide resolved
src/components/panels/HistoryListPanel.vue Show resolved Hide resolved
Signed-off-by: Patrick Stainbrook <[email protected]>
Copy link
Contributor

Language file analysis report:

File Missing Keys Unused Keys
en.json 0 0

</v-btn>
</template>
<v-tooltip top>
<template #activator="{ on, attrs }" v-if="selectedJobs.length">
Copy link
Member

Choose a reason for hiding this comment

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

i would move the v-if from the template to the v-tooltip, because the empty tooltip is not needed, when the button is also hidden.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants