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

[Designer] Add theme and container size drop-down menus #7741

Merged
merged 22 commits into from
Sep 10, 2022

Conversation

anna-dingler
Copy link
Contributor

@anna-dingler anna-dingler commented Aug 3, 2022

Related Issue

Progress towards #7540

Description

Add infrastructure for including ContainerSize and ColorTheme menus for individual containers.

If a container has the feature enabled, there will be two new drop-down menus to select the container size and theme.

When you select a size or theme from the menus, the respective properties are updated in the Host Data Payload Editor. If the host data payload is updated, we reflect the change in the drop-down menus as well.

How Verified

Verified manually on the AdaptiveCards Designer site.

Future Work

  1. Allow display of all size options at once. There will be a fourth option, View all, for the container size choice picker.
    Open question: How will this behave with the designer surface?
  2. Consolidate Microsoft Teams and Cortana Skills containers

Outstanding discussion

  • We can update the data bind variable names if we do not want to use size and theme
Microsoft Reviewers: Open in CodeFlow

@ghost
Copy link

ghost commented Aug 3, 2022

Hi @anna-dingler. Thanks for helping make the AdaptiveCards JS renderer + tooling better. As additional verification, once the JS build succeeds, please go to the test site to test out your website/designer changes.

@anna-dingler anna-dingler marked this pull request as ready for review August 5, 2022 17:28
@ghost ghost added the no-recent-activity label Aug 11, 2022
@ghost
Copy link

ghost commented Aug 11, 2022

Hi @anna-dingler. This pull request has had no recent activity for the past 5 days . Please take the necessary actions (review, address feedback or commit if reviewed already) to move this along.

@ghost ghost removed the no-recent-activity label Aug 11, 2022
@ghost
Copy link

ghost commented Aug 11, 2022

Staleness reset by anna-dingler

@ghost ghost added the no-recent-activity label Aug 17, 2022
@ghost
Copy link

ghost commented Aug 17, 2022

Hi @anna-dingler. This pull request has had no recent activity for the past 5 days . Please take the necessary actions (review, address feedback or commit if reviewed already) to move this along.

@ghost ghost removed the no-recent-activity label Aug 17, 2022
@ghost
Copy link

ghost commented Aug 17, 2022

Staleness reset by anna-dingler

Copy link
Member

@jwoo-msft jwoo-msft left a comment

Choose a reason for hiding this comment

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

Can you make the new size & theme menu always visible even though the host app doesn't have sizes and theme?
Screen Shot 2022-08-17 at 10 56 54 AM
Screen Shot 2022-08-17 at 10 57 31 AM

It was hard to notice the change until I used berlin container.
I think putting the two menu options together with host app selection menu option as a group will improve a11y since users don't have to move through all of the other buttons to change size and etc.

@anna-dingler
Copy link
Contributor Author

Can you make the new size & theme menu always visible even though the host app doesn't have sizes and theme? Screen Shot 2022-08-17 at 10 56 54 AM Screen Shot 2022-08-17 at 10 57 31 AM

It was hard to notice the change until I used berlin container. I think putting the two menu options together with host app selection menu option as a group will improve a11y since users don't have to move through all of the other buttons to change size and etc.

@jwoo-msft I can definitely move the drop down menus to be directly after the host container selection menu.

In terms of visibility, I was following the behavior of our other menus such as Emulate device: . If we make them always visible, do you think we should disable the control when a host doesn't have size or theme?

@jwoo-msft
Copy link
Member

Can you make the new size & theme menu always visible even though the host app doesn't have sizes and theme? Screen Shot 2022-08-17 at 10 56 54 AM Screen Shot 2022-08-17 at 10 57 31 AM
It was hard to notice the change until I used berlin container. I think putting the two menu options together with host app selection menu option as a group will improve a11y since users don't have to move through all of the other buttons to change size and etc.

@jwoo-msft I can definitely move the drop down menus to be directly after the host container selection menu.

In terms of visibility, I was following the behavior of our other menus such as Emulate device: . If we make them always visible, do you think we should disable the control when a host doesn't have size or theme?

that's good point, as long as the menus are together, I don't think making them always visible necessary. my main concern is that after host app is chosen, it's hard to noticed that the new menu are added presently.
if we make them always visible. would it make sense to show no entries?

@anna-dingler
Copy link
Contributor Author

Can you make the new size & theme menu always visible even though the host app doesn't have sizes and theme? Screen Shot 2022-08-17 at 10 56 54 AM Screen Shot 2022-08-17 at 10 57 31 AM
It was hard to notice the change until I used berlin container. I think putting the two menu options together with host app selection menu option as a group will improve a11y since users don't have to move through all of the other buttons to change size and etc.

@jwoo-msft I can definitely move the drop down menus to be directly after the host container selection menu.
In terms of visibility, I was following the behavior of our other menus such as Emulate device: . If we make them always visible, do you think we should disable the control when a host doesn't have size or theme?

that's good point, as long as the menus are together, I don't think making them always visible necessary. my main concern is that after host app is chosen, it's hard to noticed that the new menu are added presently. if we make them always visible. would it make sense to show no entries?

I think showing no entries might be more confusing while using other containers. We could show something along the lines of No options available or disable the control to avoid confusion there?

@jwoo-msft
Copy link
Member

Can you make the new size & theme menu always visible even though the host app doesn't have sizes and theme? Screen Shot 2022-08-17 at 10 56 54 AM Screen Shot 2022-08-17 at 10 57 31 AM
It was hard to notice the change until I used berlin container. I think putting the two menu options together with host app selection menu option as a group will improve a11y since users don't have to move through all of the other buttons to change size and etc.

@jwoo-msft I can definitely move the drop down menus to be directly after the host container selection menu.
In terms of visibility, I was following the behavior of our other menus such as Emulate device: . If we make them always visible, do you think we should disable the control when a host doesn't have size or theme?

that's good point, as long as the menus are together, I don't think making them always visible necessary. my main concern is that after host app is chosen, it's hard to noticed that the new menu are added presently. if we make them always visible. would it make sense to show no entries?

I think showing no entries might be more confusing while using other containers. We could show something along the lines of No options available or disable the control to avoid confusion there?

either options sounds good.

@paulcam206
Copy link
Member

@jwoo-msft I can definitely move the drop down menus to be directly after the host container selection menu.

In terms of visibility, I was following the behavior of our other menus such as Emulate device: . If we make them always visible, do you think we should disable the control when a host doesn't have size or theme?

I think it can make sense to either show/hide or enable/disable, but I think it depends on the broader experience. Does the theme picker work to switch between what we currently refer to as "Microsoft Teams - Light" and "Microsoft Teams - Dark"? If it does, then it certainly at least makes sense to have the theme dropdown always visible and just disabled for containers that don't have a light/dark theme pair. If the picker doesn't work, should it? If not, we should only be showing the dropdowns while displaying containers to which they apply.

@anna-dingler
Copy link
Contributor Author

@jwoo-msft I can definitely move the drop down menus to be directly after the host container selection menu.
In terms of visibility, I was following the behavior of our other menus such as Emulate device: . If we make them always visible, do you think we should disable the control when a host doesn't have size or theme?

I think it can make sense to either show/hide or enable/disable, but I think it depends on the broader experience. Does the theme picker work to switch between what we currently refer to as "Microsoft Teams - Light" and "Microsoft Teams - Dark"? If it does, then it certainly at least makes sense to have the theme dropdown always visible and just disabled for containers that don't have a light/dark theme pair. If the picker doesn't work, should it? If not, we should only be showing the dropdowns while displaying containers to which they apply.

I believe the plan is to use the theme picker for "Microsoft Teams" in the future (not implemented in this PR), so I'll enable/disable the control.

Copy link
Member

@paulcam206 paulcam206 left a comment

Choose a reason for hiding this comment

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

some minor comments/optimizations, but looks good :)

@anna-dingler anna-dingler merged commit 71aeddc into main Sep 10, 2022
@anna-dingler anna-dingler deleted the anna/consolidateContainers branch September 10, 2022 00:21
@anna-dingler
Copy link
Contributor Author

Confirmed that both of the drop-down menus will not be present when we exclude the berlin container.

michaelfarnsworth pushed a commit to michaelfarnsworth/AdaptiveCards that referenced this pull request Nov 10, 2022
* Consolidate berlin containers into one

* Add data binding

* Update class list for card host

* Update location and wording

* Disable/enable choice pickers and make values lowercase

* Move size to berlin container

* Rename variables

* Fix spacing

* Update source/nodejs/adaptivecards-designer/src/card-designer.ts

Co-authored-by: Paul Campbell <paulcam@microsoft.com>

* Add instanceof check

Co-authored-by: Paul Campbell <paulcam@microsoft.com>
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.

None yet

5 participants