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

Create new content for Apps Structure topic #1111

Merged
merged 36 commits into from
Oct 12, 2022

Conversation

neflyte
Copy link
Contributor

@neflyte neflyte commented Sep 8, 2022

Summary

New and improved content for the Apps Structure topic

Topics

  • Structure homepage
    • Manifest
    • Calls
    • Call metadata
    • Bindings
    • Static assets

This PR adds Hugo support for the following:

  • Compass Icons
    • shortcode: {{<compass-icon "icon-name" "icon description">}}
  • Improved collapsable text blocks based on the existing Bootstrap styles and Compass icons
    • shortcode: {{<collapse id="xxxx" title="yyyy">}}...{{</collapse>}}
  • External links using the {{<newtabref>}} shortcode will display an icon to indicate the link will open in a new tab
  • Note sections can optionally append a Compass Icon to the end of the title
    • shortcode: {{<note "Note Title" "icon-name" "icon desctipion">}}...{{</note>}}

@mattermod
Copy link
Contributor

Hello @neflyte,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

@neflyte neflyte self-assigned this Sep 8, 2022
@neflyte neflyte added Work in Progress Not yet ready for review preview-environment Allow the preview environment to be generated for Pull Requests coming from fork repositories and removed Contributor labels Sep 8, 2022
@github-actions
Copy link

github-actions bot commented Sep 8, 2022

Newest code from mattermod has been published to preview environment for Git SHA 0a95690

@github-actions
Copy link

github-actions bot commented Sep 8, 2022

Newest code from neflyte has been published to preview environment for Git SHA 0a95690

1 similar comment
@github-actions
Copy link

github-actions bot commented Sep 8, 2022

Newest code from neflyte has been published to preview environment for Git SHA 0a95690

…es for h4, h5, and h6 headings; Modify CSS to not break words used in links in a table cell
@github-actions
Copy link

github-actions bot commented Sep 8, 2022

Newest code from neflyte has been published to preview environment for Git SHA bd8a61f

…ctory to font directory since compass-icons.css expects its files there; Update Apps structure main topic, manifest, and calls topics; Moved call metadata (expand) to its own topic
…h build; Update Makefile dist target to not pull compass icons; Update Structure topic homepage diagram with simplified version; Update Call metadata topic
@neflyte neflyte changed the title Create new Apps Structure content Create new content for Apps Structure topic Sep 20, 2022
@neflyte neflyte removed Work in Progress Not yet ready for review preview-environment Allow the preview environment to be generated for Pull Requests coming from fork repositories labels Sep 20, 2022
@cwarnermm cwarnermm added preview-environment Allow the preview environment to be generated for Pull Requests coming from fork repositories and removed preview-environment Allow the preview environment to be generated for Pull Requests coming from fork repositories labels Oct 5, 2022
@github-actions
Copy link

github-actions bot commented Oct 5, 2022

Newest code from cwarnermm has been published to preview environment for Git SHA c0fbb81

@github-actions
Copy link

github-actions bot commented Oct 5, 2022

Newest code from neflyte has been published to preview environment for Git SHA 7379871

@github-actions
Copy link

github-actions bot commented Oct 5, 2022

Newest code from cwarnermm has been published to preview environment for Git SHA 535a8c9

Copy link
Contributor

@azigler azigler 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!

Copy link
Contributor

@mickmister mickmister left a comment

Choose a reason for hiding this comment

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

Awesome work @neflyte 👍

Holding back on approving to make sure the css changes are okay. Otherwise LGTM!

Comment on lines +3315 to +3325
h4 {
font-size: 1.25rem;
}

h5 {
font-size: 1rem;
}

h6 {
font-size: 0.75rem;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm concerned this will affect other parts of the site. @cwarnermm Do you know who owns this site, and may be able to gauge the risk of additions like this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is fixing a defect. The h4, h5, and h6 styles were all the same and made headings look incorrect.

Copy link
Member

Choose a reason for hiding this comment

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

@emdecr - Are you open to reviewing this PR's CSS changes to ensure that they won't impact other areas of the site?


- [OAuth2 Store]({{< ref "mattermost-api#apps-api" >}}) - store, expand, and retrieve user and App OAuth2 configuration data.

- [Calling other Apps]({{< ref "mattermost-api#apps-api" >}}) - use the Call API to invoke other Apps (experimental).
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if we should advertise this if it's experimental cc @levb

Copy link
Contributor Author

@neflyte neflyte Oct 6, 2022

Choose a reason for hiding this comment

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

@levb @mickmister How would anyone know if a feature of the Apps framework is "experimental" other than what's in the code or existing docs? Can we refrain from including any experimental stuff in framework releases so this isn't an issue? There's no issue with having experimental stuff in a branch.

Copy link
Member

Choose a reason for hiding this comment

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

This potentially could change with the OAuth 2.0 scopes work coming. @mickmister - We're going to move forward and merge this PR. This content can be updated iteratively as needed if you continue to be concerned.

@neflyte neflyte added preview-environment Allow the preview environment to be generated for Pull Requests coming from fork repositories and removed preview-environment Allow the preview environment to be generated for Pull Requests coming from fork repositories labels Oct 6, 2022
@github-actions
Copy link

github-actions bot commented Oct 6, 2022

Newest code from neflyte has been published to preview environment for Git SHA 2bbde8a

site/content/integrate/apps/structure/bindings/_index.md Outdated Show resolved Hide resolved
site/content/integrate/apps/structure/manifest/_index.md Outdated Show resolved Hide resolved
site/content/integrate/apps/structure/manifest/_index.md Outdated Show resolved Hide resolved
site/content/integrate/apps/structure/call/_index.md Outdated Show resolved Hide resolved
site/content/integrate/apps/structure/call/_index.md Outdated Show resolved Hide resolved
@@ -0,0 +1,60 @@
---
title: Call metadata
Copy link
Contributor

Choose a reason for hiding this comment

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

0/5 Expand(ed) call metadata? I'd really like to work the term expand into the section title here since that's what it's all about.

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 find it confusing to use the word "expand"; is the emphasis on the expansion or the resulting metadata?
I'd love to have a chat about how best to write this. 🙂

Copy link
Contributor

Choose a reason for hiding this comment

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

ping me!

| `summary` | Include key metadata for the field. |
| `id` | Include only relevant identifiers (IDs). |

### Available information
Copy link
Contributor

Choose a reason for hiding this comment

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

The "all" mode is also somewhat limited, we may want to document it, specifically for User. 0/5 on the format, but the table feels a little over the top, hard to keep track of.

Copy link
Contributor Author

@neflyte neflyte Oct 11, 2022

Choose a reason for hiding this comment

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

Unfortunately, this is how the code is written. Readers wouldn't know any of this without looking at the code, and I believe we're trying to avoid that hurdle.

We can definitely update this section when the code is updated. 🙂
Another approach might be to extract this info from the source code directly, in a similar manner to the server API reference.

@neflyte neflyte requested a review from levb October 11, 2022 16:15
@neflyte
Copy link
Contributor Author

neflyte commented Oct 11, 2022

Ready for your feedback, @levb 🙂

Copy link
Contributor

@levb levb left a comment

Choose a reason for hiding this comment

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

With comments/replies inline

@levb levb removed the 1: Dev Review Requires review by a core commiter label Oct 11, 2022
@levb
Copy link
Contributor

levb commented Oct 11, 2022

@neflyte 👍 👍 👍

@cwarnermm cwarnermm merged commit bcbdd3e into mattermost:master Oct 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Contributor preview-environment Allow the preview environment to be generated for Pull Requests coming from fork repositories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants