-
Notifications
You must be signed in to change notification settings - Fork 5
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
feat: App Guide component #415
feat: App Guide component #415
Conversation
...ges/instant-apps-components/src/components/instant-apps-app-guide/instant-apps-app-guide.tsx
Outdated
Show resolved
Hide resolved
|
||
render() { | ||
return ( | ||
<Host> | ||
<slot>It Works!</slot> | ||
<calcite-panel scale="s"> | ||
<span slot="header-content">App Guide <calcite-icon icon="lightbulb" scale="s"></calcite-icon></span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the header content going to be configurable? If so, it seems like this would be text that it can default to
App Guide
will need to be translated so I think it'd good to move the string into the t9n files and load the messages through the getMessages
util.
Here's how's it's implemented in the other components:
https://github.com/Esri/instant-apps-components/blob/master/packages/instant-apps-components/src/components/instant-apps-social-share/instant-apps-social-share.tsx#L253
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The header of the component (App Guide) will not be configurable, though the title of the content area (in bold below the divider line) will be. The app guide string has been added to the t9n file; I'll wire it up with getMessages
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the text "App Guide" going to appear in the component header? If so then I think it needs to be configurable. I don't think users will always want this text visible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kellyhutchins I'll chat with Design about this. We hadn't talked about that header being configurable or optionally hidden.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now I've added a property that will allow the header to be hidden. I've also wired up the translated strings to the header text.
...es/instant-apps-components/src/components/instant-apps-app-guide/instant-apps-app-guide.scss
Show resolved
Hide resolved
packages/instant-apps-components/src/instant-apps-app-guide.html
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Left some comments. Once addressed, i think it'd be fine to merge to the dev branch
.../instant-apps-components/src/components/instant-apps-app-guide/AppGuide/AppGuideViewModel.ts
Show resolved
Hide resolved
...nt-apps-components/src/components/instant-apps-app-guide/AppGuide/interfaces/interfaces.d.ts
Outdated
Show resolved
Hide resolved
...ges/instant-apps-components/src/components/instant-apps-app-guide/instant-apps-app-guide.tsx
Outdated
Show resolved
Hide resolved
...ges/instant-apps-components/src/components/instant-apps-app-guide/instant-apps-app-guide.tsx
Outdated
Show resolved
Hide resolved
...ges/instant-apps-components/src/components/instant-apps-app-guide/instant-apps-app-guide.tsx
Outdated
Show resolved
Hide resolved
...ges/instant-apps-components/src/components/instant-apps-app-guide/instant-apps-app-guide.tsx
Outdated
Show resolved
Hide resolved
...ges/instant-apps-components/src/components/instant-apps-app-guide/instant-apps-app-guide.tsx
Outdated
Show resolved
Hide resolved
@@ -1,3 +1,49 @@ | |||
:host { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious, was there a reason to not use ids/class names for css selectors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, the couple of factors I considered were that web component styles won't get polluted by outside stylesheets, and the simplicity of the component markup made element selectors seem reasonable.
Per our conversation, as the component evolves and new features are added I will definitely re-evaluate whether this approach needs to change in favor of more specific selectors.
Also, depending on the particulars of the testing framework, I may need to add classes or IDs to support automated testing (more on that in a future PR).
instant-apps-componentsThe following PR has been closed and merged. - link to PR If you want to publish, your Repo version should be updated and differ from the current version on NPM To publish instant-apps-components to NPMIf you would like to follow this PR up with a publish, please proceed to the actions tab and manually run the publish. |
Summary