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

Kirby list by default have divider #2671

Merged

Conversation

AgreSanGit
Copy link
Contributor

Which issue does this PR close?

This PR closes #2670

What is the new behavior?

Kirby list by default will now have divider. To have no dividers on Kirby list you now need to set showDivider to false. Update cookbook to match with new behaviour and added API description for Kirby list.

Does this PR introduce a breaking change?

  • Yes
  • No

Are there any additional context?

Checklist:

The following tasks should be carried out in sequence in order to follow the process of contributing correctly.

Reminders

  • Make sure you have implemented tests following the guidelines in: "The good: Test".
  • Make sure you have updated the cookbook with examples and showcases (for bug fixes, enhancements & new components).

Review

  • Determine if your changes are a fix, feature or breaking-change, and add the matching label to your PR. If it is tooling, dependency updates or similar, add ignore-for-release.
  • Do a self-review.
  • Request that the changes are code-reviewed
  • Request that the changes are UX reviewed (only necessary if your PR introduces visual changes)

When the pull request has been approved it will be merged to develop by Team Kirby.

@github-actions github-actions bot temporarily deployed to pr-2670-kirby-list-should-by-default-have-divider December 13, 2022 09:41 Inactive
@github-actions github-actions bot temporarily deployed to pr-2670-kirby-list-should-by-default-have-divider December 13, 2022 11:12 Inactive
},
{
name: 'getStandAloneByProperty',
description: 'Property name to decide which items should be stand alone',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe @mark-drastrup can help out here with a description, or at least verify that this is as it should be?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's basically his description from an old issue.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Top notch :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it would be more clear with a description like this: A boolean property on the item that decides if the item should stand alone. Fx. if the item is {name: 'myItem', suspiciousActivity: true }, then getStandAloneByProperty would be suspiciousActivity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed over to your new description Mark :)

@@ -13,7 +13,7 @@ export const ListItemsExampleTemplate = `<kirby-list [items]="items" [showDivide
// tslint:disable-next-line
selector: 'cookbook-list-items-example',
template: `
<kirby-page title="Items">
<kirby-page title="Items with no dividers">
Copy link
Collaborator

@RasmusKjeldgaard RasmusKjeldgaard Dec 13, 2022

Choose a reason for hiding this comment

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

I would prefer if we can rename the example components so items is the one with dividers, and the one used in the no dividers list will be called ListItemsNoDividersExampleComponent or whatever makes sense with the naming strategy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay I have changed that, but that caused several changes to routing module in example and etc which I changed too. But I have kept the same file name.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the file name of this one should be changed too, to no-dividers.
Sorry, just think we should be consistent around this so we are not confusing ourselves down the line.

@github-actions github-actions bot temporarily deployed to pr-2670-kirby-list-should-by-default-have-divider December 13, 2022 13:00 Inactive
@@ -330,7 +330,7 @@ export const routes: Routes = [
},
{
path: 'with-items',
Copy link
Collaborator

Choose a reason for hiding this comment

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

path should probably be no-dividers now, and then we should just quickly verify that any references to with-items is changed too, if they exist. Hopefully they dont 🤞

@github-actions github-actions bot temporarily deployed to pr-2670-kirby-list-should-by-default-have-divider December 14, 2022 10:28 Inactive
@github-actions github-actions bot temporarily deployed to pr-2670-kirby-list-should-by-default-have-divider December 14, 2022 12:25 Inactive
@github-actions github-actions bot temporarily deployed to pr-2670-kirby-list-should-by-default-have-divider December 15, 2022 08:47 Inactive
@AgreSanGit AgreSanGit enabled auto-merge (squash) December 15, 2022 10:22
@AgreSanGit AgreSanGit merged commit aea3223 into develop Dec 15, 2022
@AgreSanGit AgreSanGit deleted the enhancement/2670-kirby-list-should-by-default-have-divider branch December 15, 2022 11:19
kodeaben pushed a commit that referenced this pull request Dec 19, 2022
@RasmusKjeldgaard RasmusKjeldgaard added the feature Add this PR to the changelog as a feature label Jan 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Add this PR to the changelog as a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Enhancement] Kirby list should by default have divider
4 participants