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

Add list view filter to recent sessions home page component. #376

Merged
merged 14 commits into from
Dec 16, 2020

Conversation

jjbennett
Copy link
Contributor

@jjbennett jjbennett commented Dec 11, 2020

Critical Changes

Changes

Issues Closed

New Metadata

Deleted Metadata

Definition of Done

Refer to Asteroids DoD document to see any additional details for the items below

  • Any net new LWC work has Sa11y tests & 50% or above lines JEST test coverage
    CRUD/FLS is enforced in Apex
    Permission sets are updated to account for CRUD|FLS|Tab|Classes
    Field sets are updated to account for new fields
  • UX approval or UX not necessary
  • Link the pull request and work item by PR comment and Chatter post respectively, e.g. GUS: W-0000000: Work Name (Reorganize code; use custom iterator #303)
  • All acceptance criteria have been met
    • Developer
    • Code Reviewer
    • QA
      PR contains draft release notes
  • QE story level testing completed

@jjbennett jjbennett added the wip Work in progress label Dec 11, 2020
@jjbennett jjbennett changed the base branch from master to feature/attendance December 11, 2020 14:43
<language>en_US</language>
<protected>true</protected>
<shortDescription>Warning when list view limit is reached.</shortDescription>
<value>The selected list view has 2000+ records, all matching records may not be displayed.</value>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lbeller - good morning, we have a warning message that will be displayed if the list view selected to filter the recent sessions component contains 2000+ records. Screen shot attached still pending UX approval. Thanks!
Screen Shot 2020-12-11 at 8 44 25 AM

Copy link

Choose a reason for hiding this comment

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

@jjbennett , thanks! Here's suggested wording: There are over 2,000 records. Some records may not be shown.

@jjbennett jjbennett changed the title wip: Add list view filter to recent sessions home page component. Add list view filter to recent sessions home page component. Dec 11, 2020
@jjbennett
Copy link
Contributor Author

jjbennett commented Dec 11, 2020

W-8530184

@jjbennett jjbennett added ready for review and removed wip Work in progress labels Dec 11, 2020
<language>en_US</language>
<protected>true</protected>
<shortDescription>Warning when list view limit is reached.</shortDescription>
<value>The selected list view has 2000+ records, all matching records may not be displayed.</value>
Copy link

Choose a reason for hiding this comment

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

@jjbennett , thanks! Here's suggested wording: There are over 2,000 records. Some records may not be shown.

this.pageToken = this.previousPageToken;
}

set options(data) {
Copy link
Contributor

Choose a reason for hiding this comment

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

we haven't used non-@api setters like this much...if at all. neato!

Copy link
Contributor

@bethbrains bethbrains left a comment

Choose a reason for hiding this comment

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

A few questions!

Comment on lines 11 to 19
<label class="slds-form-element__label slds-m-bottom_none">
{labels.listView}
</label>
<div class="slds-form-element__control">
<c-list-view-selector
object-api-name={objectApiName}
onselect={handleListViewSelected}
></c-list-view-selector>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

need to semantically link the label to the element with a for= and an id=

</lightning-layout-item>
<lightning-layout-item>
<div class="slds-form-element slds-var-p-left_small">
<label class="slds-form-element__label slds-m-bottom_none">
Copy link
Contributor

Choose a reason for hiding this comment

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

same. link the label to the element.

</lightning-layout-item>
</lightning-layout>
</div>
<div class="slds-var-p-around_small" style="background-color:#f3f2f2">
Copy link
Contributor

Choose a reason for hiding this comment

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

style in markup? is a there a reason it's not in the css file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Laziness lol, no other explanation I can find :)


import userId from "@salesforce/user/Id";
import pmmFolder from "@salesforce/resourceUrl/pmm";
import LIST_VIEW_LABEL from "@salesforce/label/c.List_Views";
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you make this const plural LIST_VIEWS_LABEL for consistency?

Comment on lines +40 to +43
if (this.rounded) {
slds += " slds-box";
}

Copy link
Contributor

Choose a reason for hiding this comment

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

is this part of the scoped notif spec?

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 don't think so, UX wanted rounded corners.

@@ -0,0 +1,10 @@
.listViewSelector button,
Copy link
Contributor

Choose a reason for hiding this comment

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

curious, why a separate file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doesn't appear to be necessary, moving it. Thanks for the catch!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually since they are overriding lwc base components it does need to be an override.

Copy link
Contributor

Choose a reason for hiding this comment

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

but you pulled them out of the other static resource file? can't we keep them all in one file?

Copy link
Contributor

Choose a reason for hiding this comment

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

future-proofing. got it.

Copy link
Contributor

@bmuniswamy bmuniswamy left a comment

Choose a reason for hiding this comment

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

Just one comment. I was joking on the other one.

pageToken: "$pageToken",
})
listView({ error, data }) {
if (data && data.records) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@jjbennett , No early return here.. Just j/k

<lightning-layout-item padding="horizontal-small">
<lightning-button-icon-stateful
icon-name="utility:pin"
alternative-text={labels.pinListView}
Copy link
Contributor

Choose a reason for hiding this comment

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

@jjbennett , Can we add a hover text too so the users can see it just like the standard list view.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, thanks for the suggestion!

Copy link
Contributor

@bethbrains bethbrains left a comment

Choose a reason for hiding this comment

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

good work! off to QE.

@@ -0,0 +1,10 @@
.listViewSelector button,
Copy link
Contributor

Choose a reason for hiding this comment

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

but you pulled them out of the other static resource file? can't we keep them all in one file?

@@ -0,0 +1,10 @@
.listViewSelector button,
Copy link
Contributor

Choose a reason for hiding this comment

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

future-proofing. got it.

@rmonica08 rmonica08 added in QE review in QE review and removed ready for QE labels Dec 16, 2020
@rmonica08 rmonica08 merged commit 05da799 into feature/attendance Dec 16, 2020
@rmonica08 rmonica08 deleted the feature/attendance__listview-filter branch December 16, 2020 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in QE review in QE review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants