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(opacity-checkerboard): adding new component #3416

Merged
merged 12 commits into from
Aug 16, 2023
Merged

Conversation

vjosyula
Copy link
Contributor

@vjosyula vjosyula commented Jul 5, 2023

Description

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.

@vjosyula vjosyula requested a review from pfulton July 5, 2023 01:22
@vjosyula vjosyula marked this pull request as draft July 5, 2023 01:23
@github-actions
Copy link

github-actions bot commented Jul 5, 2023

Tachometer results

Currently, no packages are changed by this PR...

@Westbrook
Copy link
Contributor

Is there an issue where you’ve outlined the goals, API, and usage of an addition like this?

@vjosyula vjosyula requested review from Westbrook and jenndiaz July 17, 2023 14:15
@Westbrook
Copy link
Contributor

There's still no issue or consumption instructions in this change. Please add that somewhere before I can productively review these changes.

@jenndiaz
Copy link
Contributor

I worked on the CSS for this component, just wanted to share the context I have. It is not in the design docs site but is in contributions (https://spectrum-contributions.corp.adobe.com/page/opacity-checkerboard-beta/)

This component is used by Swatch, ColorSlider, Thumbnail, and ColorHandle to display alpha transparency.
Here is the PR on spectrum-css to add and implement the component: adobe/spectrum-css#1916

@Westbrook
Copy link
Contributor

@jenndiaz based off of the info in that PR, I'm not sure that building this as a custom element is the right call here. This seems like it is more of a shared CSS rule(s) rather than shared functionality.

@vjosyula could you look into leveraging this similar to how the clear-button package or the various icon styles? We've got Office Hours this Wednesday at 8am ET, if you wanted to discuss that path more closely.

@vjosyula vjosyula force-pushed the venkateshjo/CSS-547 branch 2 times, most recently from f5c341c to d2460bd Compare July 20, 2023 08:59
Copy link
Contributor

@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.

Great start. If we had docs, I'd be happy to merge to main with the couple of notes here. A project branch path with actual implementation to educate the documentation process also works great here. Feel free to take which ever path you're more comfortable with here.

packages/opacity-checkerboard/package.json Outdated Show resolved Hide resolved
packages/opacity-checkerboard/README.md Outdated Show resolved Hide resolved
packages/opacity-checkerboard/src/spectrum-config.js Outdated Show resolved Hide resolved
@vjosyula vjosyula requested a review from Westbrook July 21, 2023 05:06
@vjosyula vjosyula force-pushed the venkateshjo/CSS-547 branch from d2460bd to f47bc22 Compare July 21, 2023 05:28
@vjosyula vjosyula force-pushed the venkateshjo/CSS-547 branch from 23e852e to 53c4923 Compare July 27, 2023 04:03
@vjosyula vjosyula marked this pull request as ready for review July 27, 2023 23:09
@Rajdeepc
Copy link
Contributor

Rajdeepc commented Aug 9, 2023

@Westbrook I have updated the docs with the usage part to the actual documentation. Let me know if you are expecting any more changes on this.

Rajdeepc
Rajdeepc previously approved these changes Aug 9, 2023
@Westbrook
Copy link
Contributor

My question here is do we find this page helpful to a developer looking to leverage this new feature?

For instance, in #3507 it's noted that this feature is not currently leveraged in that pattern. If we were to ask the contributor there to leverage this, would the above link be enough for them to do so?

@Rajdeepc
Copy link
Contributor

I noticed clear button is not added as a part of the SWC components list, is there any specific reason we are adding opacity Checkerboard?
If we want to add this in the component list, we can create an example like Clear button inside either of ColorHandle, ColorSlider, Swatch and Thumbnail or in all of them and add it as usage examples.
Some specs will be required to achieve this.
cc @jnjosh

@Westbrook
Copy link
Contributor

Most times when there is documentation missing (which is too often), it happens because no one ever asked for it loud enough. I'd much rather documentation be missing than not really usable, but the line there is much more personal that I can really define. If we chose to exclude this from the docs, we could remove the README.md, and then the contents are no longer at question.

I'd like to say that I specifically chose not to include Clear Button in the docs because "it's only ever used in the Clear Button element", but I'd be lying if I said I had thought that far ahead. The CSS values are not documented, and if we intended for them to be used in apps, then we should absolutely get an issue open to expand the docs to include that. What I can say, the Clear Button and Close Button package are primarily left overs from a time when we couldn't consume more than one CSS package in a single SWC package (previously they were all part of the Button package from CSS, which is why the custom element definitions are where they are). It might be time to deprecate those packages and resume consume the CSS for them directly in Button, where Clear Button and Close Button are documented as custom elements.

As a consumer of SWC, I do know that we've copy/pasted the previous opacity CSS into other projects in the past, which points to me wanting the contents of this package documented at some point. I don't want to block this from moving forward, so I'll follow your lead here, either remove the README.md so the package is not included in the docs or expand the README.md so that usage instructions and the like are included, and we can get this merged. Sound good?

@Rajdeepc
Copy link
Contributor

Rajdeepc commented Aug 10, 2023

Most times when there is documentation missing (which is too often), it happens because no one ever asked for it loud enough. I'd much rather documentation be missing than not really usable, but the line there is much more personal that I can really define. If we chose to exclude this from the docs, we could remove the README.md, and then the contents are no longer at question.

I'd like to say that I specifically chose not to include Clear Button in the docs because "it's only ever used in the Clear Button element", but I'd be lying if I said I had thought that far ahead. The CSS values are not documented, and if we intended for them to be used in apps, then we should absolutely get an issue open to expand the docs to include that. What I can say, the Clear Button and Close Button package are primarily left overs from a time when we couldn't consume more than one CSS package in a single SWC package (previously they were all part of the Button package from CSS, which is why the custom element definitions are where they are). It might be time to deprecate those packages and resume consume the CSS for them directly in Button, where Clear Button and Close Button are documented as custom elements.

As a consumer of SWC, I do know that we've copy/pasted the previous opacity CSS into other projects in the past, which points to me wanting the contents of this package documented at some point. I don't want to block this from moving forward, so I'll follow your lead here, either remove the README.md so the package is not included in the docs or expand the README.md so that usage instructions and the like are included, and we can get this merged. Sound good?

Yup I think as this opacity-checkerboard can be used in above mentioned components, keeping the README.md and create a usage section will be helpful and a future proof solution. I will get this plan out to @jnjosh and have someone from the team start working on this. Thanks
@Westbrook Is it okay to merge this without README.md and then create a parallel issue to work on the usage documentation?

Copy link
Contributor

@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.

Please move this from the packages directory to the tools directory.

packages/opacity-checkerboard/src/spectrum-config.js Outdated Show resolved Hide resolved
packages/opacity-checkerboard/README.md Outdated Show resolved Hide resolved
packages/opacity-checkerboard/README.md Outdated Show resolved Hide resolved
packages/opacity-checkerboard/package.json Outdated Show resolved Hide resolved
@Rajdeepc Rajdeepc requested a review from Westbrook August 16, 2023 08:04
Copy link
Contributor

@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.

Made some nuanced changes to the docs site to better outline what this does for others.

Also, it seems like you never ran yarn to install the package and build the spectrum-*.css file, but I'm not sure.

See the changes here.

Thanks for getting this together! Hopefully this means that we can see this leveraged in some patterns, soon?

@Westbrook Westbrook force-pushed the venkateshjo/CSS-547 branch from a7cfc75 to abd9f07 Compare August 16, 2023 20:55
@Westbrook Westbrook merged commit 90202f9 into main Aug 16, 2023
@Westbrook Westbrook deleted the venkateshjo/CSS-547 branch August 16, 2023 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants