Skip to content

[Code]improve file tree#27129

Closed
WangQianliang wants to merge 2 commits intoelastic:feature/codefrom
WangQianliang:ql-improve-file-tree
Closed

[Code]improve file tree#27129
WangQianliang wants to merge 2 commits intoelastic:feature/codefrom
WangQianliang:ql-improve-file-tree

Conversation

@WangQianliang
Copy link
Contributor

@WangQianliang WangQianliang commented Dec 13, 2018

Summary

  • implement design styles
  • do not request parent node after first request file tree
  • do not send request when node is already exist

screen shot 2018-12-18 at 3 08 37 pm

@elasticmachine
Copy link
Contributor

💔 Build Failed

@WangQianliang WangQianliang changed the base branch from feature/merge-code to feature/code December 18, 2018 06:18
@WangQianliang WangQianliang requested a review from a team as a code owner December 18, 2018 06:24
@WangQianliang WangQianliang changed the title improve file tree [Code]improve file tree Dec 18, 2018
@elasticmachine
Copy link
Contributor

💔 Build Failed

implement design styles;optimize api call
@elasticmachine
Copy link
Contributor

💔 Build Failed

@snide
Copy link
Contributor

snide commented Dec 18, 2018

Hey @elastic/codesearch . I know this project is super early and y'all are just building out initial version. Please do me a favor though and try to use the Sass variables from EUI in any of your Sass files.

It makes the styling layer maintainable so that we can do broad color or sizing changes across the product. Doing this kind of thing after the fact is usually pretty painful.

Yell if you need a primer.

https://elastic.github.io/eui/#/guidelines/sass

@mw-ding
Copy link
Contributor

mw-ding commented Dec 18, 2018

@snide Hey Dave, yes, this makes perfect sense. We already have this in mind as you can see from here. Basically, we are trying to import the sass variables into the typescript because we rely on styled-component to embed css into the code.

At this moment, this may not be thorough in all the places in our codebase, because we just recently adopt this mindset. Along the way, this is definitely the be guideline in terms of styling.

@WangQianliang Qianliang, can you try to import and apply those variables as much as you can? Thanks.

@snide
Copy link
Contributor

snide commented Dec 18, 2018

Yeah, at least in the files I've seen you're actually using Sass files, so it should just be a one-liner to import the EUI sass scope.

I'd be cautious with styled components. We're likely going to move to CSS modules in EUI early next year (mostly at the urging of APM, who wants to move off of styled components) which should solve a lot of the needs people want from CSS in JS, but still provide the sass layer so the styling code can still have some manner of global scoping. I can set something up if you want some background.

The variables are only half the battle with EUI (If you're just using the JSON). There are mixins for accessibility, shadows, and all manner of things that get used all through EUI and then get imported and reused in Kibana.

Anyways, I know this is all super early so I don't want to slow you all down. Just something to keep in mind.

elastic/eui#1335

@mw-ding
Copy link
Contributor

mw-ding commented Dec 20, 2018

@snide Good to know the entire context. Thanks, Dave. Will definitely keep this in mind.

@mw-ding mw-ding added the :Code label Dec 20, 2018
Copy link
Contributor

@mw-ding mw-ding left a comment

Choose a reason for hiding this comment

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

LG

vertical-align: middle;
`;

const FolderOpenTriangle = styled.span`
Copy link
Contributor

Choose a reason for hiding this comment

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

FolderOpenTriangle seem to have a couple of shared properties with FolderClosedTriangle. You can use

const FolderOpenTriangle = styled(FolderClosedTriangle)``

to inherit and override some of them.

@WangQianliang WangQianliang changed the base branch from feature/code to feature/merge-code December 21, 2018 06:27
@WangQianliang WangQianliang changed the base branch from feature/merge-code to feature/code December 21, 2018 06:27
@WangQianliang
Copy link
Contributor Author

#27653

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@WangQianliang WangQianliang deleted the ql-improve-file-tree branch April 24, 2019 10:34
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.

5 participants