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

Mgt-get and Template improvements #252

Merged
merged 20 commits into from
Feb 28, 2020
Merged

Mgt-get and Template improvements #252

merged 20 commits into from
Feb 28, 2020

Conversation

nmetulev
Copy link
Contributor

@nmetulev nmetulev commented Jan 23, 2020

PR Type

  • Feature

Description of the changes

This PR introduces new features for both the mgt-get component and our templating implementation. These changes improve the prototyping experience for building UI with the graph.

mgt-get

  1. Added the polling-rate attribute. When set to a number of milliseconds, mgt-get will poll the request uri for updates in the defined interval. If using a resource that supports delta queries, polling will always query the delta api. The ui will only update if the data changes.

  2. Added the refresh() method to refresh the data manually. By default, the UI will only update if the data changes. Pass true to the method to force the component to update.

  3. Added the value template. Use the value template instead of the default template when expecting the response from the graph to contain an array of items - such as messages, files, users, etc. The value template will automatically be repeated for each item in the value array. The value template will also start rendering the items as soon as they are ready and doesn't have to wait for all the items to load (unlike the default template).

Templates

  1. Relaxed the requirements for double or triple curly braces. Now both {{expression}} and {{{expression}}} act the same. In addition, expressions in attributes (such as data-for or data-if) now work with or without curly braces.

  2. Added support for event and property binding in templates.

    <template>
        <button data-props="{{@click: myEvent, myProp: value}}"></button>
    </template>

    Where myEvent is added as additional context or is passed by the component. Ex:

    document.querySelector('mgt-get').templateContext = {
        myEvent: (e, context, root) => {
            // e - event args
            // context - data context for the template
            // root - root element of the template 
        }
    }

Deprecated MgtBaseComponent.templateConverters

and added MgtBaseComponent.templateContext instead. With support of events in templates, templateContext better represents the function than templateConverters and it avoids adding yet another propertiy.

PR checklist

  • Project builds (npm run build) and changes have been tested in supported browsers (including IE11)
  • All public classes and methods have been documented
  • Added appropriate documentation
  • License header has been added to all new source files (npm run setLicense)
  • Contains NO breaking changes

Documentation added here: https://github.com/microsoftgraph/microsoft-graph-docs/commit/35096f5da1758f5e75907f15306db95eea7eb869 under the mgt-1.2 branch

@shweaver-MSFT
Copy link
Contributor

This is a pretty big PR. It'll take some time to go through the code and test all the features you added. In the future, I'd love to see smaller PRs for individual features so we can review them separately, as well as clear testing instructions for each.

@nmetulev
Copy link
Contributor Author

nmetulev commented Feb 6, 2020

Agree, this would have been easier it was two separate PRs. Luckily, most of the code in the PR is sample code

Copy link
Contributor

@shweaver-MSFT shweaver-MSFT left a comment

Choose a reason for hiding this comment

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

@nmetulev thanks for walking me through the account creation and permissions setup. The flow you described makes more sense to me now. Please document this in the future to make it easier for other members of the team to quickly and easily provide feedback on the code and approach, without fighting with environment and configuration issues.

I'm not entirely sure what your expectations are for the views, but I see content in all of them, so I'll assume that means passing. Perhaps adding a comment header to each sample would bring clarity on its purpose and expected functionality.

Test results:

  • ✔ mgt-get/email
  • ✔ mgt-get/grouping-email
  • ✔ mgt-get/presence
  • ✔ mgt-get/teams
  • ❌ agenda-template - See comment for details
  • ✔ templates

The rest of the code looks good, but I don't have any real way to tell what changes correlate to which feature. Please be more granular with PRs in the future.

samples/examples/agenda-template.html Outdated Show resolved Hide resolved
@ghost ghost added the Needs: Author Feedback Issue needs response from issue author label Feb 13, 2020
@ghost ghost removed the Needs: Author Feedback Issue needs response from issue author label Feb 14, 2020
@nmetulev
Copy link
Contributor Author

Thanks @shweaver-MSFT.

I've moved most samples to storybook so now it should be easier to test and validate. Could you take a look again and make sure you are not running into any issues still?

I didn't move two samples (getTeamsMessages and presence) to storybook because #302 is causing issues with running them, so for now they will remain in the samples folder until issue is resolved.

@beth-panx, FYI, I moved some of the other stories to the templates story just to keep all the templating stories in the same place

@nmetulev
Copy link
Contributor Author

Copy link
Contributor

@shweaver-MSFT shweaver-MSFT left a comment

Choose a reason for hiding this comment

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

Looks good :) Love the storybook additions

@nmetulev nmetulev merged commit f377092 into master Feb 28, 2020
@nmetulev nmetulev deleted the nmetulev/mgt-get-updates branch February 28, 2020 23:15
shweaver-MSFT pushed a commit that referenced this pull request Jun 8, 2020
* added support for polling in mgt-get, events support in templates

* Added refresh() and polling-rate now works for any api

* relaxed requirements for {{}} and converters

* mgt get now can render both default and value templates in the order they have been defined

* fixed bad merge resolution

* fixed bug in template where square brackets were not recognized in expression. Fixed bug in template where additional context was not passed for data for

* fixed bug in template where square brackets were not recognized in expression. Fixed bug in template where additional context was not passed for data for

* added data-props support and removed @event support

* formatting

* adding get stories

* updated get stories

* moved template sample to template story

Co-authored-by: Nicolas Vogt <nicolasjvogt@gmail.com>
shweaver-MSFT pushed a commit that referenced this pull request Jun 10, 2020
* added support for polling in mgt-get, events support in templates

* Added refresh() and polling-rate now works for any api

* relaxed requirements for {{}} and converters

* mgt get now can render both default and value templates in the order they have been defined

* fixed bad merge resolution

* fixed bug in template where square brackets were not recognized in expression. Fixed bug in template where additional context was not passed for data for

* fixed bug in template where square brackets were not recognized in expression. Fixed bug in template where additional context was not passed for data for

* added data-props support and removed @event support

* formatting

* adding get stories

* updated get stories

* moved template sample to template story

Co-authored-by: Nicolas Vogt <nicolasjvogt@gmail.com>
shweaver-MSFT pushed a commit that referenced this pull request Jun 10, 2020
* added support for polling in mgt-get, events support in templates

* Added refresh() and polling-rate now works for any api

* relaxed requirements for {{}} and converters

* mgt get now can render both default and value templates in the order they have been defined

* fixed bad merge resolution

* fixed bug in template where square brackets were not recognized in expression. Fixed bug in template where additional context was not passed for data for

* fixed bug in template where square brackets were not recognized in expression. Fixed bug in template where additional context was not passed for data for

* added data-props support and removed @event support

* formatting

* adding get stories

* updated get stories

* moved template sample to template story

Co-authored-by: Nicolas Vogt <nicolasjvogt@gmail.com>
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.

None yet

3 participants