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

Restructure dashboard files and folders #9969

Merged

Conversation

stacey-gammon
Copy link
Contributor

The current arrangement of folders and files in dashboard I find very unintuitive and difficult to find anything. Here is a new proposed version, trying to be in line with the LIFT principals

Previous collapsed folder structure:
screen shot 2017-01-19 at 3 56 55 pm

New collapsed folder structure:
screen shot 2017-01-19 at 3 56 46 pm

Previous expanded folder structure:
screen shot 2017-01-19 at 3 58 20 pm

New expanded folder structure:
screen shot 2017-01-19 at 3 56 38 pm

Changes:

  • Locating our code is easy - Removing angular-specific names and grouping by function, not type.
  • Identify code at a glance - Grouping by function, not type.
  • Flat structure as long as we can. When you get to 7+ files, begin considering separation. - Removed nesting when unnecessary (e.g. component\panel\lib) and the new folder structure keeps everything to less than 7 files per folder.
  • Try to Stick to DRY - Got rid of redundant _dashboard suffix for top_nav components.

saved_dashboard section could probably be improved, but wasn't sure on a better name.

@kobelb
Copy link
Contributor

kobelb commented Jan 19, 2017

I really like it, I like the grouping by 'category' as opposed to 'type' a lot. I'm eager to hear what other people think!

avoid duplicate ‘dashboard’ naming when unnecessary
@BigFunger
Copy link
Contributor

I like these changes. It's still a bit different than what we are going with in Watcher, but about what we can accomplish without major rewrites to these files.

@w33ble
Copy link
Contributor

w33ble commented Jan 19, 2017

I also dig this. We took this one step further with the top-level index.js that would expose any "public" modules, but this is close enough for now. Much improved.

The only additional changes I might consider:

  • move /index.js to /dashboard.js
  • move /index.html to /dashboard.html
  • move /styles/main.less to simply /dashboard.less

But this it totally fine to ship as-is too. LGTM!

@stacey-gammon
Copy link
Contributor Author

I dig @w33ble, suggestions added.

@uboness
Copy link

uboness commented Jan 20, 2017

I dig @w33ble, suggestions added.

I'm not sure I'm digging this... isn't it a common practice and convention to have the module main js file named index.js? why not follow it? and if we do follow it, I suggest we do it with all "main files" (index.html and index.less)

One advantage of having the main files named <module_name>.<ext> is discoverability in the codebase. That said, this is a decision we need to make across the board and follow a clear convention we put in place.

/cc @epixa

@kobelb
Copy link
Contributor

kobelb commented Jan 20, 2017

FWIW, I'm partial to the index convention as well.

@stacey-gammon
Copy link
Contributor Author

I like being explicit, but agree it's not the current convention, and don't feel strongly either way, so, I un-dig. 😸

@uboness
Copy link

uboness commented Jan 20, 2017

BTW, I'm not partial either way... I just want to make sure we have a clear convention and we follow it. I like being explicit too... (and it has its merits as I mentioned) and if the majority likes that too we can make it the convention, but it needs to be a clear and consistent about it.

@uboness
Copy link

uboness commented Jan 20, 2017

btw... as a side note... per previous discussion I had with different ppl. The advantage of using index.js is that externally never import files, but just modules:

import { Dashboard } from '/dashboard'

vs

import { Dashboard } from '/dashboard/dashboard.js'

may look like a small thing, but it's actually bigger than it looks. by only importing the module, you force all exports to be explicit within the index.js file. And by convention have file imports only allowed within the module. This is something we can potentially lint as well.

@stacey-gammon stacey-gammon merged commit 0f0981c into elastic:master Jan 20, 2017
elastic-jasper added a commit that referenced this pull request Jan 20, 2017
Backports PR #9969

**Commit 1:**
Restructure dashboard files and folders

* Original sha: 1c01e78
* Authored by Stacey Gammon <gammon@elastic.co> on 2017-01-19T20:48:58Z

**Commit 2:**
Try to stay DRY

avoid duplicate ‘dashboard’ naming when unnecessary

* Original sha: 4e85e79
* Authored by Stacey Gammon <gammon@elastic.co> on 2017-01-19T20:56:20Z

**Commit 3:**
rename index => dashboard and add an index.js with imports

rename style

* Original sha: 423a272
* Authored by Stacey Gammon <gammon@elastic.co> on 2017-01-20T13:44:05Z

**Commit 4:**
un-dig

* Original sha: e29bf87
* Authored by Stacey Gammon <gammon@elastic.co> on 2017-01-20T14:18:27Z
@stacey-gammon
Copy link
Contributor Author

FYI - for introducing a dashboard landing page, I'm probably going to have:

  • index.js (stripped down to minimum routing)
  • dashboard.js
  • dashboard.html
  • listing\dashboard_listing.js
  • listing\dashboard_listing.html

Similar to how it is in @cjcenizal's visualize landing page PR.

@w33ble
Copy link
Contributor

w33ble commented Jan 20, 2017

btw... as a side note... per previous discussion I had with different ppl. The advantage of using index.js is that externally never import files, but just modules

Exactly. The proposal I've heard, and we've been following with watcher, is to use both.

  • index.js exports the "public" API - export { Dashboard } from './dashboard';
    • If there is more to export, perhaps from other files, it happens here as well
  • The actual code lives in dashboard.js, so it's easy to find and open
  • Any helper code that is specific to dashboard lives in this folder as well, grouped by concern
  • Using it still works the same: import { Dashboard } from 'app/dashboard';
    • Simple example with multiple exports from the index.js, import { Dashboard, DashboardPanel } from 'app/dashboard';

This isn't the place for this discussion though, I'm planning to open another issue so we can discuss and come to an agreement as a team.

@uboness
Copy link

uboness commented Jan 20, 2017

moving the actual dashboard impl. to dashboard.js makes sense... I would just not get rid of index.js

and I believe this to some degree what @stacey-gammon did (except the import part :))

stacey-gammon added a commit to stacey-gammon/kibana that referenced this pull request Jan 20, 2017
* Restructure dashboard files and folders

* Try to stay DRY

avoid duplicate ‘dashboard’ naming when unnecessary

* rename index => dashboard and add an index.js with imports

rename style

* un-dig
stacey-gammon added a commit that referenced this pull request Jan 23, 2017
* Restructure dashboard files and folders

* Try to stay DRY

avoid duplicate ‘dashboard’ naming when unnecessary

* rename index => dashboard and add an index.js with imports

rename style

* un-dig
@stacey-gammon stacey-gammon deleted the dashboard-folder-restructure branch April 6, 2017 11:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants