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

feat(coachmark): rename "sp-coachmark" to "sp-coachmark-indicator", add "sp-coachmark" #3639

Merged
merged 178 commits into from Feb 8, 2024

Conversation

Rajdeepc
Copy link
Contributor

@Rajdeepc Rajdeepc commented Sep 12, 2023

Description

Coach-Indicator and Coachmark[NEW] has been combined to be used in tandom.
Design: https://spectrum-contributions.corp.adobe.com/page/coach-mark-beta/

Related issue(s)

Motivation and context

How has this been tested?

  • Test case 1
    1. Go here
    2. Do this
  • Test case 2
    1. Go here
    2. Do this

Screenshots (if appropriate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Chore (minor updates related to the tooling or maintenance of the repository, does not impact compiled assets)

Checklist

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • If my change required a change to the documentation, I have updated the documentation in this pull request.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have reviewed at the Accessibility Practices for this feature, see: Aria Practices

Best practices

This repository uses conventional commit syntax for each commit message; note that the GitHub UI does not use this by default so be cautious when accepting suggested changes. Avoid the "Update branch" button on the pull request and opt instead for rebasing your branch against main.

@github-actions
Copy link

github-actions bot commented Sep 12, 2023

Tachometer results

Chrome

action-bar permalink

Version Bytes Avg Time vs remote vs branch
npm latest 477 kB 128.25ms - 130.58ms - unsure 🔍
-0% - +2%
-0.63ms - +2.21ms
branch 472 kB 127.81ms - 129.43ms unsure 🔍
-2% - +0%
-2.21ms - +0.63ms
-

action-button permalink

Version Bytes Avg Time vs remote vs branch
npm latest 507 kB 219.66ms - 223.14ms - slower ❌
0% - 2%
0.65ms - 4.54ms
branch 502 kB 217.93ms - 219.68ms faster ✔
0% - 2%
0.65ms - 4.54ms
-

action-group permalink

Version Bytes Avg Time vs remote vs branch
npm latest 528 kB 136.16ms - 138.25ms - unsure 🔍
-4% - +1%
-5.17ms - +0.96ms
branch 524 kB 136.42ms - 142.19ms unsure 🔍
-1% - +4%
-0.96ms - +5.17ms
-

action-menu permalink

Version Bytes Avg Time vs remote vs branch
npm latest 630 kB 251.41ms - 255.43ms - unsure 🔍
-0% - +1%
-1.05ms - +3.65ms
branch 625 kB 250.90ms - 253.34ms unsure 🔍
-1% - +0%
-3.65ms - +1.05ms
-

alert-dialog permalink

Version Bytes Avg Time vs remote vs branch
npm latest 444 kB 132.04ms - 134.65ms - faster ✔
27% - 29%
50.93ms - 53.87ms
branch 441 kB 185.06ms - 186.43ms slower ❌
38% - 41%
50.93ms - 53.87ms
-

button-group permalink

Version Bytes Avg Time vs remote vs branch
npm latest 433 kB 129.44ms - 131.67ms - unsure 🔍
-1% - +1%
-1.94ms - +1.07ms
branch 428 kB 129.97ms - 132.00ms unsure 🔍
-1% - +1%
-1.07ms - +1.94ms
-

button permalink

Version Bytes Avg Time vs remote vs branch
npm latest 438 kB 146.78ms - 148.74ms - unsure 🔍
-2% - +0%
-3.20ms - +0.07ms
branch 433 kB 148.02ms - 150.63ms unsure 🔍
-0% - +2%
-0.07ms - +3.20ms
-

card permalink

Version Bytes Avg Time vs remote vs branch
npm latest 502 kB 119.04ms - 121.17ms - unsure 🔍
-1% - +1%
-1.14ms - +1.69ms
branch 497 kB 118.90ms - 120.77ms unsure 🔍
-1% - +1%
-1.69ms - +1.14ms
-
Firefox

action-bar permalink

Version Bytes Avg Time vs remote vs branch
npm latest 477 kB 377.34ms - 398.98ms - faster ✔
1% - 9%
4.85ms - 36.15ms
branch 472 kB 397.36ms - 419.96ms slower ❌
1% - 9%
4.85ms - 36.15ms
-

action-button permalink

Version Bytes Avg Time vs remote vs branch
npm latest 507 kB 542.37ms - 577.67ms - unsure 🔍
-3% - +5%
-16.42ms - +29.42ms
branch 502 kB 538.90ms - 568.14ms unsure 🔍
-5% - +3%
-29.42ms - +16.42ms
-

action-group permalink

Version Bytes Avg Time vs remote vs branch
npm latest 528 kB 349.71ms - 368.09ms - unsure 🔍
-5% - +2%
-18.24ms - +9.16ms
branch 524 kB 353.28ms - 373.60ms unsure 🔍
-3% - +5%
-9.16ms - +18.24ms
-

action-menu permalink

Version Bytes Avg Time vs remote vs branch
npm latest 630 kB 566.29ms - 595.35ms - unsure 🔍
-4% - +4%
-24.27ms - +22.15ms
branch 625 kB 563.78ms - 599.98ms unsure 🔍
-4% - +4%
-22.15ms - +24.27ms
-

alert-dialog permalink

Version Bytes Avg Time vs remote vs branch
npm latest 444 kB 306.27ms - 327.37ms - faster ✔
17% - 25%
66.39ms - 103.29ms
branch 441 kB 386.52ms - 416.80ms slower ❌
20% - 33%
66.39ms - 103.29ms
-

button-group permalink

Version Bytes Avg Time vs remote vs branch
npm latest 433 kB 337.76ms - 359.12ms - unsure 🔍
-5% - +4%
-19.27ms - +12.43ms
branch 428 kB 340.15ms - 363.57ms unsure 🔍
-4% - +6%
-12.43ms - +19.27ms
-

button permalink

Version Bytes Avg Time vs remote vs branch
npm latest 438 kB 355.14ms - 376.30ms - unsure 🔍
-4% - +4%
-13.37ms - +15.09ms
branch 433 kB 355.35ms - 374.37ms unsure 🔍
-4% - +4%
-15.09ms - +13.37ms
-

card permalink

Version Bytes Avg Time vs remote vs branch
npm latest 502 kB 273.35ms - 289.73ms - unsure 🔍
-8% - +4%
-23.10ms - +10.82ms
branch 497 kB 272.83ms - 302.53ms unsure 🔍
-4% - +8%
-10.82ms - +23.10ms
-

@Rajdeepc Rajdeepc added WIP Component: Coach Mark Needs discussion Proposed UX or spec changes Discuss w/ design Needs insight from Spectrum or another designer labels Sep 15, 2023
@Rajdeepc Rajdeepc linked an issue Jan 30, 2024 that may be closed by this pull request
1 task
@Rajdeepc
Copy link
Contributor Author

Rajdeepc commented Jan 30, 2024

@Westbrook coach indicator fixes are added in raised in #3958

@Rajdeepc
Copy link
Contributor Author

Rajdeepc commented Jan 30, 2024

Storybook doesn't currently seem to render as expected?

I mistakenly reverted the coachmark css to the previous version. This is fixed now.

@Westbrook
Copy link
Collaborator

Centering looks better on the default indicator, but the quiet version still seems off...

image

@Westbrook
Copy link
Collaborator

Font family is being applied wrong somewhere. This shouldn't have serifed body copy:
image

@Rajdeepc
Copy link
Contributor Author

Rajdeepc commented Feb 1, 2024

Centering looks better on the default indicator, but the quiet version still seems off...

image

@Westbrook It would be nice if both the alignment issue are fixed from the css side. I have left a note to them.

Screenshot 2024-02-01 at 11 48 19 AM

Copy link
Collaborator

@Westbrook Westbrook left a comment

Choose a reason for hiding this comment

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

Taking a look at the font question, I think we could curcumvent any issue on our end by using:

:host {
    --spectrum-coachmark-content-font-family: var(--spectrum-font-family);
}

In the case that we wanted to fast forward a fix and get this into a mergable state.

@Westbrook Westbrook changed the title feat(coachmark): new component feat(coachmark): rename "sp-coachmark" to "sp-coachmark-indicator", add "sp-coachmark" Feb 7, 2024
@Westbrook
Copy link
Collaborator

LGTM. If you could catch up to main then we can merge this home!

@Rajdeepc
Copy link
Contributor Author

Rajdeepc commented Feb 8, 2024

LGTM. If you could catch up to main then we can merge this home!

Ready to land!

Copy link
Collaborator

@Westbrook Westbrook left a comment

Choose a reason for hiding this comment

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

Caught a package name update in the README, and then we'll merge this.

packages/coachmark/coach-indicator.md Outdated Show resolved Hide resolved
packages/coachmark/coach-indicator.md Outdated Show resolved Hide resolved
packages/coachmark/coach-indicator.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@Westbrook Westbrook left a comment

Choose a reason for hiding this comment

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

Let's go! 🚀

Copy link
Collaborator

@Westbrook Westbrook left a comment

Choose a reason for hiding this comment

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

Got excited. Something is up in CI...

@Westbrook Westbrook merged commit a94389c into main Feb 8, 2024
46 of 48 checks passed
@Westbrook Westbrook deleted the feature/coachmark-new branch February 8, 2024 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants