Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

InApp Notifications #14715

Merged
merged 6 commits into from
Apr 26, 2019
Merged

InApp Notifications #14715

merged 6 commits into from
Apr 26, 2019

Conversation

swmitra
Copy link
Collaborator

@swmitra swmitra commented Apr 22, 2019

This PR adds the required infrastructure to display App notifications created by Brackets publisher with the ability to filter applicability of the notifications based on multiple parameters. A sample notifications file (localized) hosted from S3 would look like -

{
    "notifications": [
        {
            "sequence": "1555874975721",
            "silent": false,
            "filters": {
                "platforms": ["OSX"],
                "version": "1.14",
                "locales": ["en"],
                "builds": [],
                "extensions": [],
                "lastUsed": ""
            },
            "html": "<div><span>Web development and design trends are changing at a rapid pace. We want to hear how new paradigms and tools are impacting you and the way you work!</span><a id='banner-button' href='https://adobe.ly/bracketsiosurvey'>Take the Brackets Survey</a></div>",
            "actionables": "#banner-button",
            "expiry" : "1556044200000"
        }
    ]
}

src/brackets.config.prerelease.json Outdated Show resolved Hide resolved
src/extensions/default/InAppNotifications/main.js Outdated Show resolved Hide resolved
src/extensions/default/InAppNotifications/main.js Outdated Show resolved Hide resolved
src/extensions/default/InAppNotifications/main.js Outdated Show resolved Hide resolved
src/extensions/default/InAppNotifications/main.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@shubhsnov shubhsnov left a comment

Choose a reason for hiding this comment

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

Apart from a few issues that I and Boopesh have mentioned this looks functionally solid to me.

src/extensions/default/InAppNotifications/main.js Outdated Show resolved Hide resolved
src/extensions/default/InAppNotifications/main.js Outdated Show resolved Hide resolved
@swmitra
Copy link
Collaborator Author

swmitra commented Apr 23, 2019

@boopeshmahendran and @shubhsnov Thanks a ton for reviewing this PR minutely under short notice 💯
Addressed all comments now.
@shubhsnov Please test it once more whenever possible.

float: right;
text-align: center;
width: auto;
min-width: 66px;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: some fomatting issues in this file.

@shubhsnov
Copy link
Collaborator

LGTM 👍 Tested with multiple platforms, versions etc. Everything is working fine. With this, we can target notifications in the future. Thanks, @swmitra for this brilliant contribution.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants