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

[BUG] mgt-person does not update the default template when details change #165

Closed
nmetulev opened this issue Oct 7, 2019 · 3 comments · Fixed by #186
Closed

[BUG] mgt-person does not update the default template when details change #165

nmetulev opened this issue Oct 7, 2019 · 3 comments · Fixed by #186
Labels
Area: Components bug Something isn't working Component: person good first issue Good for newcomers Hacktoberfest Hacktoberfest is a month-long celebration of open source software. Needs: Triage 🔍

Comments

@nmetulev
Copy link
Contributor

nmetulev commented Oct 7, 2019

Describe the bug
When using a template inside of an mgt-person component, I expect the template to reflect changes in the data. For example, when doing a person-query, I expect the template to update when the data is fetched from the graph. This does not always work, specifically in the example below

To Reproduce

    <mgt-agenda days="30">
        <template data-type="event">
            <div>{{event.subject}}</div>
            <ul>
                <li data-for="attendee in event.attendees">
                    <mgt-person person-query={{attendee.emailAddress.address}}>
                        <template>
                            <div>{{person}}</div>
                        </template>
                    </mgt-person>
                </li>
            </ul>
        </template>
    </mgt-agenda>

Expected behavior
The person details should update the template no mater where it is being used

Environment (please complete the following information):

  • OS: [e.g. iOS] Windows
  • Browser [e.g. edge, chrome, safari] edge beta
  • Framework [e.g. react, vue, none] none
  • Context [e.g. Microsoft Teams, SharePoint, Office Add-ins, Web] Web app
  • Version [e.g. 0.1] 1.0.0
@nmetulev nmetulev added bug Something isn't working good first issue Good for newcomers Needs: Triage 🔍 Area: Components Component: person Hacktoberfest Hacktoberfest is a month-long celebration of open source software. labels Oct 7, 2019
@nmetulev
Copy link
Contributor Author

Continuing discussion with @miqh from #177

@miqh: With the context object diffing function (i.e. equals() in your proposed snippet), do you think a simple deep comparison implemented locally is sufficient? Otherwise, if you anticipate more complex items, such as Set or RegExp, being made available in the context, it might be better to adopt a dependency which does the deep compare thoroughly.

The context will always be a simple object without any functions or complex items because the data is coming as json from the Microsoft Graph. A deep equal function implemented locally is enough. The utils module is a good place for an implementation

@miqh: I can't locate any existing tests which attempt to exercise the core component classes. It'd be nice to bolt some in to catch regressions because it seems like quite an important central piece of common code. Perhaps the APIs are too in flux at the moment?

Agreed 100%. APIs shouldn't change too much at this point and we should be testing for regressions. Fully open to suggestions and approaches.

@miqh
Copy link
Contributor

miqh commented Oct 17, 2019

@nmetulev, after I implemented a fix (#186) to address this issue, I looked into possible avenues with getting some tests around all the web components in this project. I tried to get something going locally but to no avail.

Here are some points that may be of use:

  • I noticed Jest was already set up as the test runner for this project. By default it uses JSDOM, which doesn't have web component support, nor does it intend to any time soon.
  • I looked into Puppeteer and using jest-puppeteer to see if I could make use of headless Chrome to get web component support for scaffolding tests. It seems possible, but I couldn't get it working.
  • I came across @open-wc/testing and it seems to provide some useful helpers for testing web components. It looks useful because it's also based on lit-element, which web components in this project also build upon. I checked out a local demo project and was able to get it up and running. Unfortunately, the testing framework is based on Karma and not Jest.

@nmetulev
Copy link
Contributor Author

Thanks for looking into testing @miqh . Do you think we should switch to Karma based on your findings? Jest is left over from earlier investigations we did with Stencil JS and I think we always hoped to get it running with lit-element. But we are not tied to it and it sounds like the Karma is recommended

nmetulev pushed a commit that referenced this issue Oct 18, 2019
These changes cause rendered slots to be wiped and rebuilt if the
context used for rendering has changed from the last render.
Prior to this, any context change would never go on to update the
rendered slot.
@ghost ghost removed the State: In Review label Oct 18, 2019
@ghost ghost locked as resolved and limited conversation to collaborators Feb 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Area: Components bug Something isn't working Component: person good first issue Good for newcomers Hacktoberfest Hacktoberfest is a month-long celebration of open source software. Needs: Triage 🔍
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants