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

Add lock icon for disabled and readonly states #189

Merged
merged 18 commits into from
Jun 27, 2023
Merged

Conversation

ynotdraw
Copy link
Contributor

@ynotdraw ynotdraw commented Jun 20, 2023

🚀 Description

This PR adds the lock icon for disabled and readonly states according to our designs.


🔬 How to Test


📸 Images/Videos of Functionality

Iconography

While I was in here, I adjusted some of the iconography in our demos to be a bit closer to what we'd prefer.

BEFORE AFTER
Screenshot 2023-06-20 at 7 56 19 PM Screenshot 2023-06-20 at 7 56 39 PM

Readonly + Disabled Locks

A few before and after pictures of the readonly and disabled states.

BEFORE AFTER
Screenshot 2023-06-20 at 7 52 01 PM Screenshot 2023-06-20 at 7 51 30 PM
Screenshot 2023-06-20 at 7 52 45 PM Screenshot 2023-06-20 at 7 53 03 PM
Screenshot 2023-06-20 at 7 55 04 PM Screenshot 2023-06-20 at 7 54 54 PM

@ynotdraw ynotdraw self-assigned this Jun 20, 2023
@changeset-bot
Copy link

changeset-bot bot commented Jun 20, 2023

🦋 Changeset detected

Latest commit: 4e80e9e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@crowdstrike/ember-toucan-core Patch
@crowdstrike/ember-toucan-form Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions
Copy link
Contributor

github-actions bot commented Jun 20, 2023

Preview URLs

Env: preview
Docs: https://6f071178.ember-toucan-core.pages.dev

@@ -204,7 +204,7 @@ Target the error block via `data-error`.
<div class='mb-4 w-64'>
<Form::Fields::Checkbox
>
<:label>Label <svg class="inline" xmlns="http://www.w3.org/2000/svg" width="24" height="24" stroke="currentColor" viewBox="0 0 24 24"><path d="M12 3a9 9 0 11-6.364 2.636A8.972 8.972 0 0112 3zm0 4.7v5.2m0 3.39v.01" fill="none" stroke-linecap="round" stroke-linejoin="round" stroke-width="2"></path></svg></:label>
<:label>Label <svg class="inline w-4 h-4 -mt-1" xmlns="http://www.w3.org/2000/svg" width="24" height="24" stroke="currentColor" viewBox="0 0 24 24"><path d="M12 3a9 9 0 11-6.364 2.636A8.972 8.972 0 0112 3zm0 4.7v5.2m0 3.39v.01" fill="none" stroke-linecap="round" stroke-linejoin="round" stroke-width="2"></path></svg></:label>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mentioned this in the PR description, but this is an update to our examples with the :label named block so that the icon looks not as big and bulky.

Screenshot 2023-06-20 at 7 56 39 PM

export interface ToucanLockIconComponentSignature {
Element: SVGElement;
Args: {};
Blocks: {
Copy link
Contributor

Choose a reason for hiding this comment

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

No way to get the type system to fail to check if the consumer passes a block, eh?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could be missing something somewhere, but I tried Blocks: never and glint still passed

Copy link
Contributor

Choose a reason for hiding this comment

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

Womp

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it has to be Blocks: { default: never }?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah! you were right! TIL! Thanks a bunch, @nicolechung !

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm getting this with Blocks: { default: never }:

Type 'never' must have a '[Symbol.iterator]()' method that returns an iterator.

The error seems to be coming from our use of never rather than the fact that the consumer is passing a default block. I think the error we'd want to see would read something like "Argument of type [...] is not assignable to parameter of type 'never'".

What's less bad? Not producing a typechecking error when we should be or producing a wrong and possibly confusing error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Talked over Zoom - we reverted!

Copy link
Contributor

Choose a reason for hiding this comment

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

Screenshot 2023-06-26 at 2 53 41 PM

According to the RFC it says if you have no blocks then it might work, but I'm not sure if this has been implemented.

https://rfcs.emberjs.com/id/0748-glimmer-component-signature/#blocks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants