Inline Add Button Component #4270

margaree · 2023-11-20T20:56:14Z

This is the initial work on the inline add button component. Its original purpose was for list but it now has the requirement of being a standalone component. I'll add comments throughout, but most of this is up for debate especially naming.

This does not include more detailed documentation on the README (followup story) and also doesn't include the ability to hide when not hovering (will do separately, didn't want to clutter this PR too much).

Also note that there will likely be a different icon use, which will also change on hover and focus.

github-actions · 2023-11-20T20:57:28Z

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-4270/

Note

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

margaree · 2023-11-20T20:57:12Z

components/button/button-add.js

+/**
+ * A component for quickly adding items to a specific locaiton.
+ */
+class ButtonAdd extends FocusMixin(LocalizeCoreElement(LitElement)) {


Currently have this as d2l-button-add. Including inline was thought to not be great because css-wise it's not actually inline. I looked up synonyms for inline but didn't fine anything I thought was great. Thoughts?

Also, I didn't use ButtonMixin. Let me know if there are other pieces of that which are important to a button that I'm missing.

I agree with not using ButtonMixin -- it just includes too many <button> attributes that would be confusing to handle here.

margaree · 2023-11-20T21:01:15Z

components/button/button-add.js

+	static get properties() {
+		return {
+			/**
+			 * The text to be shown in the tooltip as a hint when visible-text is false
+			 * @type {string}
+			 */
+			label: { type: String },
+			/**
+			 * The text to show in the button when visible-text is true
+			 * @type {string}
+			 */
+			text: { type: String },
+			/**
+			 * When true, show the button with icon and visible text. When false, only show icon.
+			 * @type {boolean}
+			 */
+			visibleText: { type: Boolean, attribute: 'visible-text' }


Most basic component states (demo page is also probably more useful for this):

Default -> visible-text is false, component shows the divider line, an add icon, and a tooltip which contains whatever is in label (design refers to this as "hint") OR "Add Item" as a fallback

Visible Text -> when visible-text is true, the component shows the divider, and in the centre is the plus icon and the text specified in text OR "Add Item" as a fallback.

text and label will not be used at the same time (the tooltip will not be shown if the text is visible and vice versa).

An aria description is potentially going to be used as well, I'll be discussing that more with design.

margaree · 2023-11-20T21:01:47Z

components/button/button-add.js

+	}
+
+	static get styles() {
+		return [labelStyles, css`


I didn't use buttonStyles here as it felt like it complicated things more than made them easier. Let me know if I should be using that.

margaree · 2023-11-20T21:03:08Z

components/button/button-add.js

+			<button class="d2l-label-text" id="${this._buttonId}">
+				<div class="line"></div>
+				<div class="content">
+					<d2l-icon icon="tier1:plus-default"></d2l-icon>
+					${this.visibleText
+		? html`<span>${text}</span>`
+		: html`<d2l-tooltip class="vdiff-target" offset="18" for="${this._buttonId}" for-type="label">${label}</d2l-tooltip>`}
+				</div>
+			</button>
+		`;


This implements its own button within rather than using subtle and/or icon button because we don't seem to want the focus/hover states that those provide.

margaree · 2023-11-20T21:04:03Z

components/button/button-add.js

+
+	_onClick() {
+		/** Dispatched when click happens */
+		this.dispatchEvent(new CustomEvent('d2l-button-add-click', {


I made this a component specific click event rather than just firing up click since list consumers could be listening for this and just having a generic click seemed like it could cause confusion.

That makes sense. In that case, we probably want to call e.stopPropagation();. I'm kinda surprised we aren't doing that here.

I'd like to explore this a little bit... there are lots of components that fire click events, including just clicking on a <span> or whatever, so hoping we don't need to worry about this.

Either way, does it need to bubble and cross shadow root boundaries?

It would need to bubble and cross shadow root boundaries. The plan is that to use this consumers of list would have something like:
<d2l-list add-button>...</d2l-list>
and then the button-add component would appear after each list item. We don't want consumers to have to manage adding this component themselves after each item, and behaviour is designed to be consistent (i.e., wouldn't have some list items with button-add and others that don't have it, etc.).

All that to say, for the list case, consumers of list would want to listen for this event and then handle the click of the add button however they wish to.

Gotcha. In that case, another option: since list is going to be the one rendering <d2l-button-add>, it could wire up to its click event and then fire its own d2l-list-add-button-click event.

So consumers would go <d2l-list add-button @d2l-list-add-button-click="{...}">.

I like that idea - I think that would be more straightforward for consumers.

lang/en.js

margaree · 2023-11-20T21:14:10Z

Demo page here https://live.d2l.dev/prs/BrightspaceUI/core/pr-4270/components/button/demo/button-add.html

components/button/button-add.js

dlockhart · 2023-11-22T17:46:51Z

components/button/button-add.js

+/**
+ * A component for quickly adding items to a specific locaiton.
+ */
+class ButtonAdd extends FocusMixin(LocalizeCoreElement(LitElement)) {


I agree with not using ButtonMixin -- it just includes too many <button> attributes that would be confusing to handle here.

components/button/button-add.js

dlockhart · 2023-11-22T17:54:12Z

components/button/button-add.js

+
+	_onClick() {
+		/** Dispatched when click happens */
+		this.dispatchEvent(new CustomEvent('d2l-button-add-click', {


I'd like to explore this a little bit... there are lots of components that fire click events, including just clicking on a <span> or whatever, so hoping we don't need to worry about this.

Either way, does it need to bubble and cross shadow root boundaries?

components/button/test/button-add.axe.js

components/button/test/button-add.test.js

…el and just use text, remove labelStyles, remove click event handling

…into inline-add-component

components/button/button-add.js

dlockhart · 2023-11-22T20:46:55Z

components/button/button-add.js

-			label: { type: String },
-			/**
-			 * The text to show in the button when visible-text is true
+			 * When text-visible is true, the text to show in the button. When text-visible is false, the text to show in the tooltip.
 			 * @type {string}
 			 */
 			text: { type: String },


Once the PropertyRequiredMixin stuff merges, this should likely use it.

components/button/test/button-add.vdiff.js

lang/en.js

…e translation files

dlockhart

Cool!

dlockhart · 2023-11-24T15:18:28Z

components/button/button-add.js

 	static get properties() {
 		return {
 			/**
 			 * When text-visible is true, the text to show in the button. When text-visible is false, the text to show in the tooltip.
 			 * @type {string}
 			 */
-			text: { type: String },
+			text: { type: String, required: true },


Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

d2l-github-release-tokens · 2023-11-24T15:35:59Z

🎉 This PR is included in version 2.165.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

dlockhart · 2023-11-24T22:57:51Z

components/button/button-add.js

+		const id = !this.textVisible ? this._buttonId : undefined;
+
+		return html`
+			<button class="d2l-label-text" id="${ifDefined(id)}">


Just remembered randomly: make sure you set type="button" on this, otherwise it defaults to submit and will submit forms!

Good catch, thanks! Will fix

margaree added 5 commits November 20, 2023 12:20

Button Add Component: Initial work

78583ea

add tests

5014f12

get visual diff tests working, simplify code

622671a

Fix axe tests

4bf83ae

css tweaks

c6eac4f

github-actions bot mentioned this pull request Nov 20, 2023

Updating vdiff goldens for PR 4270 #4271

Merged

margaree commented Nov 20, 2023

View reviewed changes

margaree requested review from dbatiste and jameswklassen November 20, 2023 21:15

margaree added 4 commits November 21, 2023 09:17

fix lint

96efcf0

reflect visible-text

9007154

replace padding-right

4cf9f2d

organize visual diffs

e532d55

dbatiste reviewed Nov 22, 2023

View reviewed changes

components/button/button-add.js Outdated Show resolved Hide resolved

dbatiste reviewed Nov 22, 2023

View reviewed changes

components/button/button-add.js Outdated Show resolved Hide resolved

dbatiste reviewed Nov 22, 2023

View reviewed changes

components/button/button-add.js Show resolved Hide resolved

dbatiste reviewed Nov 22, 2023

View reviewed changes

components/button/button-add.js Show resolved Hide resolved

dlockhart reviewed Nov 22, 2023

View reviewed changes

margaree added 4 commits November 22, 2023 14:38

code review feedback: rename visible-text to text-visible, remove lab…

3cc039e

…el and just use text, remove labelStyles, remove click event handling

Merge branch 'main' into inline-add-component

d9ce259

fix click event in demo, axe test, and tweak vdiff test names

b84a859

Merge branch 'inline-add-component' of github.com:BrightspaceUI/core …

dce2414

…into inline-add-component

dbatiste reviewed Nov 22, 2023

View reviewed changes

components/button/button-add.js Outdated Show resolved Hide resolved

margaree added 2 commits November 22, 2023 15:04

code review feedback: use getFocusPseudoClass

370c9cb

fix lint

a08cbd9

margaree marked this pull request as ready for review November 22, 2023 20:18

margaree requested a review from a team as a code owner November 22, 2023 20:18

dlockhart reviewed Nov 22, 2023

View reviewed changes

margaree added 4 commits November 23, 2023 07:51

code review feedback: remove unnecessary line from vdiff tests, updat…

1bc32db

…e translation files

fix lint

a8c06af

Merge branch 'main' into inline-add-component

f95a9ee

code review feedback: use PropertyRequiredMixin for text

2c0bf41

dlockhart approved these changes Nov 24, 2023

View reviewed changes

Updating vdiff goldens (#4271)

9fb99a4

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

margaree merged commit 6124689 into main Nov 24, 2023
5 checks passed

margaree deleted the inline-add-component branch November 24, 2023 15:34

d2l-github-release-tokens bot added the released label Nov 24, 2023

dlockhart reviewed Nov 24, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Inline Add Button Component #4270

Inline Add Button Component #4270

margaree commented Nov 20, 2023 •

edited

Loading

github-actions bot commented Nov 20, 2023

margaree Nov 20, 2023

margaree Nov 20, 2023

dlockhart Nov 22, 2023

margaree Nov 20, 2023

margaree Nov 20, 2023

margaree Nov 20, 2023

margaree Nov 20, 2023

dbatiste Nov 22, 2023

dlockhart Nov 22, 2023

margaree Nov 22, 2023

dlockhart Nov 22, 2023

dlockhart Nov 22, 2023

margaree Nov 22, 2023

margaree commented Nov 20, 2023

dlockhart Nov 22, 2023

dlockhart Nov 22, 2023

dlockhart Nov 22, 2023

dlockhart left a comment

dlockhart Nov 24, 2023

d2l-github-release-tokens bot commented Nov 24, 2023

dlockhart Nov 24, 2023

margaree Nov 27, 2023

Inline Add Button Component #4270

Inline Add Button Component #4270

Conversation

margaree commented Nov 20, 2023 • edited Loading

github-actions bot commented Nov 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

margaree commented Nov 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlockhart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d2l-github-release-tokens bot commented Nov 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

margaree commented Nov 20, 2023 •

edited

Loading