vmware-archive / clarity

Clarity is a scalable, accessible, customizable, open source design system built with web components. Works with any JavaScript framework, built for enterprises, and designed to be inclusive.
http://clarity.design
MIT License
6.43k stars 763 forks source link

Correcting the datagrid variable names #6601

Closed anooptp closed 2 years ago

anooptp commented 2 years ago

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

What is the current behavior?

Issue Number: #6599

What is the new behavior?

As is

Does this PR introduce a breaking change?

Other information

vmwclabot commented 2 years ago

@anooptp, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding Signed-off-by: John Doe <john.doe@email.org> to the last line of each Git commit message. The e-mail address used to sign must match the e-mail address of the Git author. Click here to view the Developer Certificate of Origin agreement.

netlify[bot] commented 2 years ago

✔️ Deploy Preview for zzz-angular-clarity-design-old ready!

🔨 Explore the source changes: 256775fba218a12ede9ef3b6d2b5ba607c017537

🔍 Inspect the deploy log: https://app.netlify.com/sites/zzz-angular-clarity-design-old/deploys/61f2b124da2d2b00088b20a6

😎 Browse the preview: https://deploy-preview-6601--zzz-angular-clarity-design-old.netlify.app

vmwclabot commented 2 years ago

@anooptp, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding Signed-off-by: John Doe <john.doe@email.org> to the last line of each Git commit message. The e-mail address used to sign must match the e-mail address of the Git author. Click here to view the Developer Certificate of Origin agreement.

vmwclabot commented 2 years ago

@anooptp, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding Signed-off-by: John Doe <john.doe@email.org> to the last line of each Git commit message. The e-mail address used to sign must match the e-mail address of the Git author. Click here to view the Developer Certificate of Origin agreement.

mathisscott commented 2 years ago

Thanks, @anooptp ! We appreciate the contribution. Last steps here would be a rebase to handle the branch conflict and a final review from another team-member.

Thanks so much!

CC: @steve-haar @hippee-lee

anooptp commented 2 years ago

Thank you @mathisscott . Have corrected the merge conflict.

anooptp commented 2 years ago

I would like to ask you to change the commit message to chore(angular:datagrid): correcting datagrid variable names for expandable

@bbogdanov Sure, I will change the commit message. Any reason for build failure...? Build logs are not clear.

mathisscott commented 2 years ago

I would like to ask you to change the commit message to chore(angular:datagrid): correcting datagrid variable names for expandable

@bbogdanov Sure, I will change the commit message. Any reason for build failure...? Build logs are not clear.

It failed a code linting/formatting check step.

When you change the commit message, run npm run format to get a list of the files that need fixing. Then run npm run format:fix path/to/file.ts on the files it returns.

anooptp commented 2 years ago

It failed a code linting/formatting check step.

When you change the commit message, run npm run format to get a list of the files that need fixing. Then run npm run format:fix path/to/file.ts on the files it returns.

I ran the above mentioned command, however it didn't list any files. Am I missing something.

image

mathisscott commented 2 years ago

Looking at the build details, it looks like it doesn't like something in this file...

projects/angular/src/data/datagrid/datagrid-row-detail.ts

Try running npm run format:fix projects/angular/src/data/datagrid/datagrid-row-detail.ts

mathisscott commented 2 years ago

@anooptp : so to fix the build, you'll need to run npm run golden:fix && npm golden:save in terminal from the root /clarity directory. this will update the file that informs us of breaking changes to the public API (our "golden" file).

after doing that, you'll want to squash the commits down again. thanks!

anooptp commented 2 years ago

@anooptp : so to fix the build, you'll need to run npm run golden:fix && npm golden:save in terminal from the root /clarity directory. this will update the file that informs us of breaking changes to the public API (our "golden" file).

thank you @mathisscott for the input

anooptp commented 2 years ago

I am getting below error:

image

mathisscott commented 2 years ago

I am getting below error:

image

I'm sorry. You ran the command I gave you but I gave you the wrong command. It should be npm run golden:fix && npm run golden:save.

Maybe that will help?

anooptp commented 2 years ago

npm run golden:fix && npm run golden:save

The first command itself is failing with the above mentioned error, and it is not executing the second one.

anooptp commented 2 years ago

I'm sorry. You ran the command I gave you but I gave you the wrong command. It should be npm run golden:fix && npm run golden:save.

Maybe that will help?

@mathisscott - do we need to run this command pre-commit or post-commit.

Currently, I am running this command with changed files in git staged on a Windows 11 machine using git bash.

Am I missing something?

Steps Performed (from clarity root directory):

  1. npm install
  2. git add .
  3. npm run format:fix (WORKED)
  4. npm run golden:fix (ERROR)
mathisscott commented 2 years ago

@anooptp I apologize for these challenges. This is usually a lot easier. Let's check to see if you have the npm package that the golden commands try to run.

Try npm view ts-api-guardian in your terminal and see if it returns anything.

If it doesn't, then try doing a fresh npm install (or just installing that package directly at version 0.6.0.

If it does, then we may be seeing an issue with git bash and I may need to step in and get this branch fixed up for you.

anooptp commented 2 years ago

Yes, ts-api-guardian is installed. PFB the screenshot:

mathisscott commented 2 years ago

This may be a git bash problem then. Something that package/script is trying to do that is not simpatico with git bash.

I'll finish this out. Thank you so much for your contribution @anooptp . I do appreciate it!

mathisscott commented 2 years ago

Ported this PR to https://github.com/vmware/clarity/pull/6669

Thanks @anooptp .

anooptp commented 2 years ago

Thank you @mathisscott for you inputs.