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

Clehner/maximize views #50

Merged
merged 20 commits into from
Feb 28, 2018
Merged

Clehner/maximize views #50

merged 20 commits into from
Feb 28, 2018

Conversation

lehnerchristian
Copy link

@lehnerchristian lehnerchristian commented Feb 26, 2018

closes #51

maximize_views

Copy link
Contributor

@thinkh thinkh left a comment

Choose a reason for hiding this comment

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

Implementation-wise it looks fine. However, I would add a margin between title and the expand/compress icon or move the icon left to the close button.

@lehnerchristian
Copy link
Author

yes, the margin is still an issue

further changes I'll make:

  • Maximize views by double clicking a tab
  • A tooltip when hovering over the expand / restore buttons
  • Probably a tooltip when hovering over the view tag indicating that it can be maximized by double clicking
  • different positioning of the buttons (in a maximized view I'll hide the close button and show the the restore button instead)

@thinkh
Copy link
Contributor

thinkh commented Feb 26, 2018

Your suggested changes sound good to me.

@lehnerchristian
Copy link
Author

suggested changes implemented:
maximize_views

@mstreit
Copy link

mstreit commented Feb 26, 2018

very nice, looks great!

@@ -19,7 +19,7 @@ export class LayoutContainerEvents {
static readonly EVENT_TAB_REORDED = 'tabReorded';
static readonly EVENT_CHANGE_ACTIVE_TAB = 'changeActiveTab';
static readonly EVENT_MAXIMIZE = 'maximize';
static readonly EVENT_MINIMIZE = 'minimize';
static readonly RESTORE_SIZE = 'restoreSize';
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't you keep the EVENT_ at the beginning of the variable name?

constructor(document: Document, public readonly build: (layout: IBuildAbleOrViewLike)=> ILayoutContainer, private readonly restorer: (dump: ILayoutDump, restoreView: (referenceId: number) => IView) => ILayoutContainer) {
super(document, {
name: '',
fixed: true
});
this.node.dataset.layout = 'root';
this.visible = true;

this.on(LayoutContainerEvents.EVENT_MAXIMIZE, (evt) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

prefer: `_evt, view'

// maximize views
const view = evt.args[0];

const section = document.createElement('section');
Copy link
Contributor

Choose a reason for hiding this comment

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

prefer: this.node.ownerDocument.createElement

@@ -18,6 +18,8 @@ export default class ViewLayoutContainer extends ALayoutContainer<IViewLayoutCon
this.node.dataset.layout = 'view';
this.node.appendChild(view.node);

this.header.insertAdjacentHTML('beforeend', `<button type="button" title="Expand view" class="size-toggle ${this.options.fixed ? 'hidden' : ''}" aria-label="Toggle View Size"><span><i class="fa fa-expand"></i></span></button>`);
Copy link
Contributor

Choose a reason for hiding this comment

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

when the layout is fixed, I cannot maximize the view?

@sgratzl sgratzl merged commit 331186c into develop Feb 28, 2018
@sgratzl sgratzl deleted the clehner/maximize_views branch February 28, 2018 09:11
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.

4 participants