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

Milestone 2: Adding data-media-background prop #18

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

tingting-lwy
Copy link

@tingting-lwy tingting-lwy commented Apr 10, 2023

Milestone 2: Adding the page-color-scheme

  • Added the property data-media-background that takes the JSON object MediaBackgroundConfig
  • Added MediaBackgroundConfig and its values to MarketplaceView
  • Created a group of fields on the Configurator for the data-media-background attribute
  • Users can test an image, colors (or multiple), and gradient types and angles

Edge Cases tested:

  • an Image URI should take precedence over colors
  • if image URI is broken/wrong, fallback color takes over
  • if no gradient type is selected and there is multiple colors, no background is used (assumed expected behavior)
  • Changing angle or gradient type has no effect on single color or image backgrounds

Here is an example of an image
image

Here is an example of a singular color:
image

Here is an example of gradient colors:
image

Here is an example of gradient colors using other CSS valid values:
image

Copy link
Contributor

@jaxonL jaxonL left a comment

Choose a reason for hiding this comment

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

works as intended, with a minor bug on the generated code.

vue/configurator/src/components/lib/WidgetProps.ts Outdated Show resolved Hide resolved
vue/configurator/src/components/Configurator.vue Outdated Show resolved Hide resolved
Copy link
Contributor

@jaxonL jaxonL left a comment

Choose a reason for hiding this comment

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

minor nits remaining. overall, seems good to merge.

config?.type === WidgetPropType.ARRAY
"
>
<label id="multipleInputs"> colors example: red, blue </label>
Copy link
Contributor

@jaxonL jaxonL Apr 13, 2023

Choose a reason for hiding this comment

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

just caught this this time around -- if there are more config props with type = ARRAY, you would have multiple labels with the same id + placeholder test. that wouldn't be good for accessibility issues.

Copy link
Author

Choose a reason for hiding this comment

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

true! I actually decided I didn't need to ID this label since any labels created by config props with type = ARRAY would then be styled using the selector label rather than creating accessibility issues by potentially using same ids

vue/configurator/src/components/Configurator.vue Outdated Show resolved Hide resolved
@@ -321,4 +409,9 @@ export default class Configurator extends Vue {
word-wrap: break-word;
overflow: auto;
}
label {
Copy link
Contributor

Choose a reason for hiding this comment

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

minor comment: if anyone were to add a label element to the configurator, it would be styled using the rules below.

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.

2 participants