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

Support secondary text in ListItem widget #1700

Closed

Conversation

aciccarello
Copy link
Contributor

@aciccarello aciccarello commented Mar 23, 2021

Type: bug / feature

The following has been addressed in the PR:

  • There is a related issue
  • All code matches the style guide
  • Unit tests are included in the PR
  • For new widgets, an entry has been added to the .dojorc
  • For new widgets, theme.variant() is added to the root domnode
  • Any widget variant uses theme.compose like this
  • WidgetProperties are exported

Description:
This adds secondary text to the ListItem widget. Because secondar text often requires more height, I've replaced the single height css class with a range of sizes. This also allows different size leading/trailing content. It's a breaking change for anyone overriding that CSS class but should allow greater flexibility.

Resolves #1688

secondary list item material theme
secondary list item dojo theme
primary list item material theme
primary list item dojo theme

@vercel
Copy link

vercel bot commented Mar 23, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

dojo.widgets – ./

🔍 Inspect: https://vercel.com/dojo/dojo.widgets/FadnaiZbuQhsF6VbdjNu51epM36C
✅ Preview: https://dojowidgets-git-fork-aciccarello-1688-secondary-list-61d513.vercel.app

widget-test-docs – ./

🔍 Inspect: https://vercel.com/dojo/widget-test-docs/CeeWryaQ88wFXjMAttferCTAfFxy
✅ Preview: https://widget-test-do-git-fork-aciccarello-1688-secondary-list-455719.vercel.app

@codecov
Copy link

codecov bot commented Mar 23, 2021

Codecov Report

Merging #1700 (abd8a9c) into master (6b9d384) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1700      +/-   ##
==========================================
+ Coverage   90.07%   90.08%   +0.01%     
==========================================
  Files          94       94              
  Lines        5046     5052       +6     
  Branches     1374     1379       +5     
==========================================
+ Hits         4545     4551       +6     
  Misses        249      249              
  Partials      252      252              
Impacted Files Coverage Δ
src/list/index.tsx 80.23% <100.00%> (+0.29%) ⬆️
src/date-input/index.tsx 91.07% <0.00%> (ø)
src/chip-typeahead/index.tsx 89.71% <0.00%> (+0.09%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6b9d384...abd8a9c. Read the comment docs.

Copy link
Member

@tomdye tomdye left a comment

Choose a reason for hiding this comment

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

@aciccarello It seems to me that this would be better implemented as different List Item types. ie. Avatar Item, Two Line Item etc. Otherwise we're just going to end up with a large number of different properties when we could have had easy to use / reason with widgets.
It would also be good to have an example of each of these item types.

src/list/index.tsx Outdated Show resolved Hide resolved
@aciccarello
Copy link
Contributor Author

@tomdye I reworked this to remove the height property that was added. Instead it depends on a mix of min-height and padding to ensure things fit well. The leading content is also given a min-width of 40px so even if you have a 24px icon it still aligns to the material spec. I did not do anything special for the two line list to float a 24px icon to the top since that would seem unexpected for users and would need another 8px of padding which wouldn't be needed if you have a 56px high leading image.

I also renamed the .primary class name to .text since it wraps both the primary and secondary text. This was added since the last release so it shouldn't be a breaking change.

Updated screenshots

I've split the example into two, one for single line and a second for two line however they both are pretty simplistic. In the committed code I'm using the location icon example for the single line example and the avatar for the two line example but a variety is shown for the sake of the PR.

list item example material theme
two line list item example material theme
list item example dojo theme
two line list item example dojo theme

In case I (or anyone else) want this manual testing example code later later:

export function differentListItemTypes({value, label = ''}: MenuOption, secondary?: string) {
	const graphicStyles = {height: '56px', color: 'white', display: 'flex', alignItems: 'center', justifyContent: 'center'};
	switch (label[0]) {
		case 'A':
			return {
				primary: label,
				secondary
			}
		case 'C':
			return {
				leading: <Icon type="locationIcon" />,
				primary: label,
				secondary,
				trailing: value
			}
		case 'D':
		case 'E':
		case 'F':
		case 'G':
			return {
				leading: <Avatar>{label[0]}</Avatar>,
				primary: label,
				secondary,
				trailing: <Icon type="starIcon" />
			}
		case 'W':
			return {
				leading: <div styles={{...graphicStyles, width: '100px', background: 'purple'}}>56 x 100</div>,
				primary: label,
				secondary,
				trailing: <Icon type="starIcon" />
			}
		default:
			return {
				leading: <div styles={{...graphicStyles, width: '56px', background: 'blue', 'fontSize': '0.875rem'}}>56 x 56</div>,
				primary: label,
				secondary,
				trailing: value
			}
	}
}

@aciccarello aciccarello requested a review from tomdye March 25, 2021 16:51
this makes the height more flexible
also rename main content wrapper class to .text since it wraps primary and secondary.
The leading content is given a min-width of 40px
For 24px leading icons, they will align to the front
This does not align leading icons to the top right on two-line list items
This is because the top/bottom padding is set to 8px instead of 16px
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.

Support secondary text in ListItem widget
2 participants