-
Notifications
You must be signed in to change notification settings - Fork 5
Conversation
* The icon class for your button. | ||
* See: https://github.com/RocketChat/Rocket.Chat/blob/develop/packages/rocketchat-theme/client/vendor/fontello/css/fontello.css | ||
*/ | ||
icon: string; |
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.
@graywolf336 I still believe icon should be more than a string. We'll be needing them in multiple places.
imho, a IUiElementFactory
could be injected (maybe in init
on the client side), e. g.
interface IUiElementFactory{
getButtonFactory(): IUiButtonFactory;
getRoomContextContainerFactory(): IRoomContextContainerFactory;
}
interface IUiButtonFactory{
createButton(identifier: string): IButton;
}
interface IUiButton{
render(): string; //could also be a DOM
}
...
On the provider side:
class FontelloButton implements IUiButton{
render(){
return `<span class="${ this.identifier }" />`
}
}
class FontelloButtonFactory implements IUiButtonFactory{
//createButton returns instance of FontelloButton
}
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.
I think this is a decent route to go, except it should be for the button's icon only.
readonly assets: IAssetExtend; | ||
|
||
/** Accessor for declaring the contextual bar buttons your Rocketlet provides. */ | ||
readonly contextualBarButtons: IContextualBarButtonsExtend; |
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.
@graywolf336 I would expect the ContextBarButtons
to be part of a client interface, not a configuration interface. With respect to assets I'm unsure. Could there be assets for the server-side?
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.
Hopefully the mobile applications will eventually allow some configuration from the Rocketlets which means the server will need to serve some pieces of them up. Assets are going to be served up by the server and if they are going to require a user be logged in before accessing the asset, then yes they need to be registered.
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 the assets being served and registered.
Still wonder whether the ContextualBar should be part of the Configuration interface or a client/ui-interface. But maybe I'm stuck in Meteor thinking.
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.
If it's going to be used by other technologies than a web browser, which I hope we eventually will allow, then yes I believe it should be.
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.
👍 I in fact meant Icon
instead of Button
. in ABAP:
REPLACE ALL OCCURENCES OF 'button` in above_comment WITH 'icon' IGNORING CASE.
However, also a button (icon+label+optional tooltip) could be created from the IUiElementFactory
. For UI-Elements, believe constructor injection is fine. Thus, a button (a real button this time) and a ContextBarElement (in lack of a better name) could be created like that (consumer side):
const myContainer = uiElementFactory.getContextBarElementFactory().createContextBarElement(
uiElementFactory.getButtonFactory(). createButton(
uiElementFactory.getIconFactory().createIcon('icon-github-circled'),
uiTextFactory.createI18nText('Github'), //somehow solve internationalization
uiTextFactory.createI18nText('Github-integration')
),
uiAssetProvider.getAsset('main.html')
);
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.
@graywolf336 is that how you imagine a consumption?
## How do the buttons operate? | ||
Fantastic question - this is not __yet__ solidified. This will be updated as this feature is finished. | ||
|
||
However, the essential idea is as such. Your Rocketlet will register the asset that is needed, it must exist in your Rocketlet package and can not be an external url. Then the ContextualBarButton is added. These two things are done server side. Then whenever a user loads the web interface and the conditions are met for your button, it will be displayed in the contextual bar. When a user clicks the button, the contextual bar's panel will slide out and your asset will be loaded inside of it. Before your asset is loaded, however, a JavaScript context will be created which contains a class which you can interact back to the server with along with a few libraries (__exact libraries to be determined__) and you will not have access to Meteor nor anything else due to the abstraction layer being applied. |
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.
@graywolf336 as far as I understood, Asset
in this context will not be an image but basically a html snippet. Is there some other asset type imaginable? If not, it might make sense to explicitly define a type of the html-asset (e. g. a DOM-node-structure or even an own dedicated interface
interface IVisualizeableAsset {
render(): DOMTree
}
class HtmlAsset implements IVisualizeableAsset{
protected getInnerHtml(): string;
}
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.
@graywolf336 afaiu (nderstood), the minimal variant of an asset could be an html with a container div and single script element which then loads a script from an external server and attaches it to the created div. Right?
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.
That might be the normal usage, however I don't want to limit a feature like this to what we can imagine right now. Which is why I am trying to keep it as a very generic asset which they provide and tell us to load.
Limited to web client and cordova mobile application. | ||
|
||
## I have never heard of this, what is it?! | ||
Great news, you're amoung great company! Contextual Bar is how Rocketlets refer to what most people know inside of Rocket.Chat as the "tab bar". However, there is currently a proposal to redesign the Rocket.Chat interface and within that design the "tab bar" idea is switching to a contextual bar style. This name also helps provide more insight as to the purpose of the buttons on the bar - they provide additional context for the users to consume. |
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.
@graywolf336 I imagined the "context bar" to be the button bar which is displayed for each message (with the message commands such as pin, star, forward etc).
That made me think about the options in general. I like your explanation why it should be called "context". What I'm not too happy about is the "bar": It implies a style of visualization. Also, there could be visualizations wanted related to the room (tab bar) as well as related to messages. What do you think about a RoomContextContainer
and a MessageContextContainer
.
@@ -0,0 +1,28 @@ | |||
import { RoomType } from '../rooms/RoomType'; | |||
|
|||
export interface IContextualBarButton { |
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.
@graywolf336 I did not understand the semantic "Button" here: As far as I got it now, this adds a complete tab (or "panel"), not only a button
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.
What would be a valid name? As IContextualBar
is what these are being added to and the button is being registered which then the button shows the panel. The registering a button is why I called it that and because I couldn't determine a better name for it.
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.
hmmm - Element, Container, Panel, Extension ... just guessing.
To me, a button is the clickable thing, an icon+label+tooltip. It's a part of a you-name-it
@graywolf336 this time I got the feeling we're not thinking in the same direction. You asked me for a review, so I commented straight out of my brain. |
@mrsimpson Yeah, I am not sure the best route to approach it right now but I just want to get something going so that we can iterate from there after it's been implemented. |
Going to hold off on merging this or anything related to the tab bar until they're done with the UI/UX refresh as they've changed several things in regards to current items. |
I am still playing with this idea, however I want to get it merged into master so I can mess with it even more in the development environment. Wanted your thoughts @mrsimpson