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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(cv-text-input-skeleton): add CvTextInputSkeleton #1542

Merged

Conversation

mateusbandeiraa
Copy link
Contributor

@mateusbandeiraa mateusbandeiraa commented Oct 11, 2023

Contributes to #167

What did you do?

Implemented cv-text-input-skeleton.

Why did you do it?

Because Carbon Vue rocks 馃槃 . And it was requested on #167.

How have you tested it?

Ran the existing unit tests and added new ones.

Were docs updated if needed?

  • N/A
  • No
  • Yes

(I updated the storybook. Is there anything else that needs to be updated?)

@mateusbandeiraa mateusbandeiraa changed the title feat(CvTextInputSkeleton): add initial skeleton implementation feat(cv-text-input-skeleton): add CvTextInputSkeleton Oct 12, 2023
@davidnixon
Copy link
Collaborator

@mateusbandeiraa I know this is stilla draft but it looks really nice. For me there are only a couple of tweaks to the storybook before this one is ready.

  1. The tag in the template in the storybook is not closed. You can see the error in the console window.
    runtime-core.esm-bundler.js:41 [Vue warn]: Template compilation error: Element is missing end tag.
    
  2. The story should hide the controls that are not relevant. I think for this one, only the hideLabel is needed.
    image

Hints

diff --git a/src/components/CvTextInput/CvTextInput.stories.js b/src/components/CvTextInput/CvTextInput.stories.js
index a262e4c7..ea687602 100644
--- a/src/components/CvTextInput/CvTextInput.stories.js
+++ b/src/components/CvTextInput/CvTextInput.stories.js
@@ -244,7 +244,7 @@ Password.parameters = storyParametersObject(
   Password.args
 );
 
-const templateSkeleton = `<cv-text-input-skeleton v-bind='args'>`;
+const templateSkeleton = `<cv-text-input-skeleton v-bind='args'/>`;
 const TemplateSkeleton = args => {
   return {
     components: { CvTextInputSkeleton },
@@ -254,6 +254,12 @@ const TemplateSkeleton = args => {
 };
 
 export const Skeleton = TemplateSkeleton.bind({});
+Skeleton.parameters = {
+  controls: {
+    // exclude everything except `hideLabel`
+    exclude: ['helperText', 'invalidMessage', 'etc'],
+  },
+};
 Skeleton.parameters = storyParametersObject(
   Skeleton.parameters,
   templateSkeleton,

@mateusbandeiraa mateusbandeiraa marked this pull request as ready for review October 15, 2023 01:01
@mateusbandeiraa
Copy link
Contributor Author

Instead of excluding everyting other than hideLabel, I usedinclude: ['hideLabel'].

@mateusbandeiraa
Copy link
Contributor Author

I'm having a hard time writing the accessibility test. This always pass, not sure if it is working:

it('is accessible', async () => {
  const skeleton = render(CvTextInputSkeleton);
  expect(skeleton.container).toBeAccessible('cv-text-input-skeleton');
})

@davidnixon
Copy link
Collaborator

Instead of excluding everything other than hideLabel, I usedinclude: ['hideLabel'].

I din't know "include" was a thing. That would probably help me clean up some of my own tests. Thanks. Nice!

@davidnixon
Copy link
Collaborator

I'm having a hard time writing the accessibility test. This always pass, not sure if it is working:

it('is accessible', async () => {
  const skeleton = render(CvTextInputSkeleton);
  expect(skeleton.container).toBeAccessible('cv-text-input-skeleton');
})

This works great for me.

  it('is accessible', async () => {
    const main = document.createElement('main');
    const skeleton = render(CvTextInputSkeleton, {
      container: document.body.appendChild(main),
    });
    await expect(skeleton.container).toBeAccessible('cv-text-input-skeleton');
  }, 10000)

To verify it is working I added a bogus role to the span like:

    <span
      v-if="!hideLabel"
      role="this is bogus"
      :class="[`${carbonPrefix}--label`, `${carbonPrefix}--skeleton`]"
    >

And then the test produces:

Error: Scan: cv-text-input-skeleton

  • Message: The role 'this,is,bogus' defined on the element is not valid per ARIA specification

I removed the bogus role and the test runs cleanly.

@mateusbandeiraa
Copy link
Contributor Author

@davidnixon I don't plan doing any more changes to this PR if that looks good for you.

Copy link
Collaborator

@davidnixon davidnixon left a comment

Choose a reason for hiding this comment

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

LGTM

@kodiakhq kodiakhq bot merged commit c3469f5 into carbon-design-system:main Oct 19, 2023
5 checks passed
@mateusbandeiraa mateusbandeiraa deleted the cv-text-input-skeleton branch October 19, 2023 00:39
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.

None yet

2 participants