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

Inspiration demo controls #5157

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

ChrisLabattD2L
Copy link
Contributor

This is an attempt at adding controls to demo components based on things set in the demoProperties field.

Currently I've only added support for:

  • Boolean values
  • Text Input
  • Calendar Input
  • Select Input (dropdown of options)

@ChrisLabattD2L
Copy link
Contributor Author

Somehow the changes I made in another branch for status indications for pulled into this, so disregard those 😅

Copy link
Contributor

Thanks for the PR! 🎉

We've deployed an automatic preview for this PR - you can see your changes here:

URL https://live.d2l.dev/prs/BrightspaceUI/core/pr-5157/

Note

The build needs to finish before your changes are deployed.
Changes to the PR will automatically update the instance.

Comment on lines +174 to +183
static get demoProperties() {
return {
type: { type: 'select', options: ['default', 'critical', 'success', 'warning'], label: 'Alert Type' },
subtext: { type: 'text', label: 'Subtext' },
buttonText: { type: 'text', attribute: 'button-text', label: 'Button Text' },
hasCloseButton: { type: 'switch', attribute: 'has-close-button', label: 'Has Close Button' },
noPadding: { type: 'switch', attribute: 'no-padding', label: 'No Padding' }
};
}

Copy link
Contributor

Choose a reason for hiding this comment

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

This is awesome!

I don't want to make it more complex but a future idea might be allowing you to see the different types within the same demo or something. I still think there is merit in certain properties being shown simultaneously for comparison purposes

Copy link
Contributor

Choose a reason for hiding this comment

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

I also am still having internal debates on this being part of the component vs part of the demo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I had a similar thought as well, but had decided to make it internal in the end, just so that it was all in the same place. I'd initially added these fields to the actual properties, but decided to change that since doing it like this would also allow the user to choose the order of things rather than have them be alphabetical order or anything like that (since properties are usually in alphabetical order)

I'm not sure I understand what you mean by seeing the different types within the same demo though?

Copy link
Contributor

@BhamelinD2L BhamelinD2L Nov 15, 2024

Choose a reason for hiding this comment

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

Like I mentioned in our presentation, I think there's an argument to be made that showing varieties can have value sometimes, but specific configurations should be demo-adjustable.

For example, having a click me to do x button is utilitarian (Do you need this feature or not?), but deciding which type of alert, or if you need a normal vs subtle button, may be a softer problem that having choices on-screen at the same time can help solve.

With that in mind, something like this would be extremely cool:
image
But I don't think that's a problem we will solve for now :D

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