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

Generic toolbar buttons #207

Merged
merged 21 commits into from
May 11, 2021

Conversation

fschmenger
Copy link
Collaborator

@fschmenger fschmenger commented Apr 29, 2021

This adds a generic toggle button as a Wegue component, which replaces redundant implementations of each toggle button for the individual modules. As a consequence, the modules icon resource now has to be declared in app-conf.json.

The management of visibility states has to be changed to a global event bus based implementation, as the existing concept no longer works with inheritance.
…n resources in app-conf.json.

As a side effect the orientation of the toolbar menu is currently vertical.
…f the module has changed, which can be consumed by the parent class. This is more convenient than listening to the event on the global event bus and fixes a bug in AttributeTableWin, when the global event was consumed by the parent class before the actual module visibility is toggled.
@fschmenger
Copy link
Collaborator Author

Some further notes:

  • Now that the hamburger menu uses the same color as the toolbar, the 'darkLayout' attribute could be assigned once as a global option, rather than specifying it for every toolbar-button. We could work on this in conjunction with Use Vuetify Theming #202.
  • Is the 'text' property of the toolbar-buttons a valid option, which is in use in some of the projects? This currently looks ugly, so we could think about either removing it or using it as a tool-tip text.
  • Removing individual toolbar-button implementations forces us to declare a button icon attribute for each module in app-conf.json. In a forthcoming feature branch I'm routing this attribute to the windows (module cards) too for consistency. So we could possibly remove the default icon declaration from the individual windows.

Copy link
Collaborator

@chrismayer chrismayer left a comment

Choose a reason for hiding this comment

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

LGTM, thnaks for this @fschmenger. I added 2 minor (non-blocking) comments you might want to address before merging

@@ -24,26 +24,29 @@
<component
:is="tbButton.type" :key="index"
:icon="tbButton.icon" :text="tbButton.text"
:color="color"
:dark="tbButton.dark"
:dark="tbButton.dark" :moduleName="tbButton.moduleName"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Bad indentation

/>
</template>

<!-- slot to inject components after the auto-generated buttons (by config) -->
<slot name="wgu-tb-after-auto-buttons"></slot>

<v-menu v-if="menuButtons.length" offset-y>
<v-menu v-if="menuButtons.length" offset-y nudge-bottom="15">
<!--v-slot="activator" to v-slot:activator="{on}" -->
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this line be removed?

Copy link
Collaborator Author

@fschmenger fschmenger May 11, 2021

Choose a reason for hiding this comment

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

Regarding this change:

One implication of wrapping buttons by a toggle-group is, that the orientation of the hamburger menu is now vertical. So I inserted a little offset such that it is not showing on top of the toolbar. IMO it doesn’t look very nice without it, but I can certainly remove it.

Copy link
Collaborator

@chrismayer chrismayer May 11, 2021

Choose a reason for hiding this comment

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

My question was with regard to the commented line. Not the line above. The vertical menu is fine.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The commented line was not introduced by me.
If you ask me it can be certainly removed. I can do that now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh sorry - didn't want to blame you 😉 . Yes, would be cool, if you could remove this line.

@chrismayer
Copy link
Collaborator

Thanks for your ongoing effort @fschmenger. This is good to go now.

For transparency: There will be some more PRs tackling some refactoring tasks. The last step will be to adjust the unit tests at once to avoid duplicate work, so it is a know issue that this (and some follow ups) will break the test suite of the master branch for a couple days.

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

Successfully merging this pull request may close these issues.

2 participants