Skip to content

Conversation

Anemy
Copy link
Member

@Anemy Anemy commented Jun 2, 2021

COMPASS-4860 also gets COMPASS-3921

There's a large diff since we're moving some things around, so I'll leave some comments in places where things have changed.
Here are some of the changes:

  • Moved compass-collection-ddl and compass-database-ddl into one package, currently it's named databases-collections, if y'all think of another name please let me know!
  • Updated the create database and create collection modals to use leafygreen components. They do look a bit different now, still syncing with Claudia to finalize.
  • Updated the electron environment for running the collections and databases views and modals in isolated development (see screenshot at the bottom).
  • Updated how the create database modal was showing fields for collection properties so it now shares this with the create collection modal.
  • Built on Maurizio's work on how the form for creating collections works, adding the collection-fields components. This involves removing a number of things from the store and moving them into this new component's state.

Some notes/thoughts for possible future things:

  • We're adding in some leafygreen components here, which updates the look of components which exist in other forms in other places in Compass. This means we'll be losing some design consistency in the short term, until we have more things using these components. We should have a common place for these components (this has been [hadron-react-components](https://github.com/mongodb-js/compass/tree/master/packages/hadron-react-components/src) in the past). Probably something we want to discuss as a team on how we want to structure those, and style them (css in js with emotion and typescript? keep them how they are?compass-components?).
  • The collections-table and databases-table have a similar toolbar, however it's styles are duplicated. It would be nice to have this shared. Probably a Toolbar component could do us good.
  • The collation component is used by compass-indexes. We aren't touching that part in this work, but it would be nice to reduce that code duplication (maybe when we make all of these modals look similar).
  • Maurizio noticed in the first pr for this work is that an error is returned when creating collections with custom collation on only in the development environment (this works successfully in production/compass in video lower down):

Screen Shot 2021-06-02 at 4 07 36 PM

Vids


Create db with collection with collation and time series

createDBWithTS.mp4

Create collection with time series

createCollectionEx.mp4

Screenshot of create db when connected to < 5
Screen Shot 2021-06-02 at 5 05 28 PM

Still working on a few tests for collection-fields.jsx and finalizing styles w/ Claudia, I figured this could be open while that's going on since I won't get to it till later in the day EU. done

Since this is a new package, and we are removing old packages, we'll need to release on npm and update Compass' plugins when we're comfortable with these changes/ready to merge.

@Anemy Anemy mentioned this pull request Jun 2, 2021
10 tasks
appRegistry.registerRole('Global.Modal', DROP_DATABASE_ROLE);
appRegistry.registerStore('DatabasesPlugin.DatabasesStore', DatabasesStore);
appRegistry.registerStore('DatabasesPlugin.CreateDatabaseStore', CreateDatabaseStore);
appRegistry.registerStore('DatabasesPlugin.DropDatabaseStore', DropDatabaseStore);
Copy link
Member Author

Choose a reason for hiding this comment

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

Here's where we combined where the two plugins register to Compass.

}
}

export default class CollectionFields extends PureComponent {
Copy link
Member Author

Choose a reason for hiding this comment

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

Here's the CollectionFields component where we render the options for creating collections (shared w/ create database).
The more general components above are things we probably want to have in a generalized place in Compass where we can also pull the theme.

/>
{this.renderCollectionNameRequiredNotice()}
{this.renderError()}
</ConfirmationModal>
Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

@gribnoysup gribnoysup left a comment

Choose a reason for hiding this comment

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

Overall looks really neat! A few things caught my eye so I left some comments here and there

};
}

renderDatabaseNameField() {
Copy link
Collaborator

@gribnoysup gribnoysup Jun 10, 2021

Choose a reason for hiding this comment

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

Feel free to ignore this as a nit, but I would suggest to avoid using render* methods on class components and instead break this into multiple components where those internal state values are props and callbacks: it give you smaller testing units, more composition opportunities, and from personal experience (so of the least importance 😄) I find this way of writing components leading to better props contracts (which means more understandable components) because you clearly see all component dependencies at a glance when writing them like that. They don't even need to be pure or memoized, just plain function components

Copy link
Member Author

@Anemy Anemy Jun 10, 2021

Choose a reason for hiding this comment

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

Good thinking. I haven't done much straight function usage with react in the past, I like the sound of it. Updated the PR to use some. Updated to use controlled components too. Also broke out the two generic components, collapsible-field-set and field-set, which will make the next step of putting them in a more central place for Compass (let's chat about this next few days maybe - my current thinking is having a compass-components that's typescript which has generic components and light leafygreen wrappers. it would be plugged into a store or some global configurable for styles).

@Anemy
Copy link
Member Author

Anemy commented Jun 10, 2021

@gribnoysup pushed some updates if you'd like to take a look.

  • Moved some of the component parts into functions, see comment above for more.
  • Updated the collations components, so now the new styles are shared with the create-index-modal in the compass-indexes package. (tested making a new index with collation and it seems to work - screenshot at bottom [1])
  • Updated the license (removed it from the package and removed the old license part from the README)

Before merging we can make sure we're pulling in any recent changes to dependency versions of package dependencies for these two repos we are merging from the main branch.

[1]
Screen Shot 2021-06-10 at 7 08 33 PM

Copy link
Collaborator

@gribnoysup gribnoysup left a comment

Choose a reason for hiding this comment

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

Absolutely awesome work 🔥 Thanks for addressing those comments!

If you want any help with merging/releasing this thing, I'm happy to pair on it (we can test the whole thing locally before publishing to remote)

@gribnoysup gribnoysup merged commit 9bab8f3 into master Jun 11, 2021
@gribnoysup gribnoysup deleted the COMPASS-4860 branch June 11, 2021 13:26
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