-
Notifications
You must be signed in to change notification settings - Fork 234
COMPASS-558 Add DDL buttons #745
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
Conversation
b01eee6
to
08b7485
Compare
Design wise it makes sense if on clicking the create database/collection modal it also opened the databases/collections table in the background. So that you can see the new entry being created. If that makes it any easier.... |
I agree with you that only having the action available when in the right place is a bit odd. Adding some ToDo's:
|
should be fixable with an
should use
That needs to be changed. I see two possibilities:
My preference is for 1. because they are shortcuts. Additionally, the newly created db/collection should be selected after creation (just as if the user clicked on it in the sidebar) so the user can instantly start working with this new collection. In order to make this work, it may be necessary to move the modal definitions to the "Home" level, so that they can be opened on any screen after connecting. @pzrq Can you make tickets in the DDL Manipulation epic for the outstanding issues? The refactor to open the modals from anywhere seems like the most pressing one. Maybe you can start working on that, and we put COMPASS-558 in Blocked state in the mean time. It will probably roll over into the new sprint. |
08b7485
to
6cfb4d7
Compare
I prefer #1 too.
…On 16 January 2017 at 19:47, Thomas Rückstieß ***@***.***> wrote:
If you use the keyboard Enter key to submit, modals refresh the whole page
should be fixable with an onSubmit() handler and a evt.preventDefault().
Can probably done quickly in this PR.
(Possibly related) Code smell from relative imports, e.g. const
CollectionsActions = require('../../../database/lib/actions/collections-
actions');
should use app.appRegistry.getActions(...). Can probably also be handled
right now.
Buttons do not work if you are not in the right place, i.e.
That needs to be changed. I see two possibilities:
1. Open modal regardless where you are.
2. Navigate to the respective screen (e.g. instance level for add
database) and open the modal there
My preference is for 1. because they are shortcuts. Additionally, the
newly created db/collection should be selected after creation (just as if
the user clicked on it in the sidebar) so the user can instantly start
working with this new collection.
In order to make this work, it may be necessary to move the modal
definitions to the "Home" level, so that they can be opened on any screen
after connecting.
@pzrq <https://github.com/pzrq> Can you make tickets in the *DDL
Manipulation* epic for the outstanding issues? The refactor to open the
modals from anywhere seems like the most pressing one. Maybe you can start
working on that, and we put COMPASS-558 in Blocked state in the mean time.
It will probably roll over into the new sprint.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#745 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AA-E-_1p-wyuY54V8WJEBeEk4qogVYnIks5rTA-OgaJpZM4LkPTr>
.
--
*{* name : "*Sam Weaver*",
title : "Product Manager",
phone : "+447549938422",
location : ["New York City, NY", "London, UK"],
twitter : ["@Samuel_Weaver <https://www.twitter.com/samuel_weaver>", "
@mongodb <https://www.twitter.com/mongodb>"],
facebook : "MongoDB <https://www.facebook.com/mongodb>" *}*
|
7e21e43
to
694c66e
Compare
694c66e
to
2017b08
Compare
d1d1e9f
to
87030c0
Compare
Depending on your preference, I'm happy to handle any and all style on this after its merged, Peter. |
Thanks @fredtruman will take you up on that, definitely at least for @samweaver's request:
EDIT: COMPASS-705 |
Implementing: https://mongodb.invisionapp.com/share/6E9XJUCKW#/screens/213327458 The arrow needs to be moved to the left side of the menu to replace it in a future commit.
Designers can style pass later, e.g. not sure how to do the fade gradient in the top right instance area, the active parent child thing more cleanly or the specific circled plus icon inside another circle from: https://mongodb.invisionapp.com/share/6E9XJUCKW#/screens/213327444
87030c0
to
4195fb8
Compare
Took me an hour with console.log() and console.trace() to finally realise what was really going on here.
bf395ca
to
664ca29
Compare
664ca29
to
5da030e
Compare
After fighting COMPASS-635, COMPASS-690, COMPASS-706 and COMPASS-707 for the last hour, going to just give in, set this This PR can be reviewed independently of the Travis issues above, and can be merged by Compass adminstrators (or we could stoop to restarting individual builds too). |
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.
LGTM, could use a style pass to move the create database icon to the bottom of the sidebar.
@KeyboardTsundoku Please let @fredtruman know on COMPASS-705 👍 |
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.
Very nice, and unobtrusive (I feared it would be cluttered but the fact that the symbols only show on the hovered row avoids that).
Some small changes (I added the styling ones to COMPASS-705):
Require code changes:
- Add a
-is-disabled
BEM class for all icons/button so that the styling can be different for disabled items. - Some tooltips include the database/collection name, some don't. I think it's pretty clear which database/collection is meant (it's the highlighted row, and you get another confirmation in the modal), so I would suggest to remove all database/collection names from tooltips and keep them very short and concise:
- Create Collection
- Drop Collection
- Drop Database
- The tooltip on the big "Create Database" button on the bottom is unnecessary, it just repeats what the button says.
Styling:
- The tooltips should not be translucent, they should be opaque like all other tooltips.
- The contrast is a little low on non-active rows (darker gray). The icons are barely visible.
- For the create database icon in the "Instance Details View" at the top, I did not see that at all (contrast = 0). I only noticed it when going through the code :-)
- on secondaries, the icons should not light up when hovering. The user should get the impression the icons are disabled.
- on secondaries, the "Create Database" button should somehow be muted to indicate it's not active (disabled).
/> | ||
<ReactTooltip id={TOOLTIP_IDS.CREATE_COLLECTION} /> | ||
<i | ||
className="compass-sidebar-icon compass-sidebar-icon-drop-database fa fa-trash-o" |
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.
Let's pass in an additional BEM modifier class compass-sidebar-icon-is-disabled
or similar when on secondary nodes so that @fredtruman can style disabled items differently.
This should be done on all create/drop icons and the big create database button
renderCreateDatabaseButton() { | ||
const isWritable = app.dataService.isWritable(); | ||
const tooltipText = isWritable ? | ||
'Create database' : |
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.
Big button does not need a tooltip "Create Database", because it just repeats what is on the button already. Only show the "...is not available..." tooltip when secondary.
const collectionName = this.getCollectionName(); | ||
const isWritable = app.dataService.isWritable(); | ||
const tooltipText = isWritable ? | ||
`Drop ${this.props.database}.${collectionName} collection` : |
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 "Drop Collection"
'data-tip': createTooltipText | ||
}; | ||
const dropTooltipText = isWritable ? | ||
`Drop ${this.props._id} database` : |
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 "Drop Database"
Also disambiguate other tooltip parameters.
Looks good, nice job! |
Awesome, merging. Thanks @rueckstiess @KeyboardTsundoku @fredtruman @samweaver 👍 |
Design Mock Up
https://mongodb.invisionapp.com/share/6E9XJUCKW#/screens/213327470
Screenshots
NOTE: Expect some changes in style pass ticket COMPASS-705.
New 5 buttons (enabled, e.g. primary, mongos)
New 5 buttons (disabled, e.g. secondary, arbiter)
Tasks Done
Other bugs and refactoring blocking this PR:
Create database
modal only displays on click when already at the instance levelCreate collection
modal only displays on click when already at the database levelDrop database
modal only displays on click when already at the instance levelDrop collection
modal only displays on click when already at the database levelPlease feel welcome to push commits, test, review and add or check off these TODOs:
const CollectionsActions = require('../../../database/lib/actions/collections-actions');
From Sam:
From Sam & Thomas: