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

Add custom style for files/dirs matching core:ignoredNames #1048

Merged
merged 4 commits into from Feb 28, 2018

Conversation

Projects
None yet
3 participants
@viddo
Copy link
Contributor

viddo commented Mar 25, 2017

Git-ignored files/dirs have the custom class status-ignored which gives the entry a more subtle color indicating that it's ignored. This PR contains a change that yields the same result for matches of core:ignoredNames setting:

ignored-dir-and-files

I.e. if the tree-view config setting tree-view:hideIgnoredNames is set to false it displays the entries but with the custom style, too.

Extracted the ignoredPatterns from the tree-view into a separate object (IgnoredNames), which is now reused for both the Directory and File model. The git status of an ignored entry still have precedence over the ignored status.

Alternate Designs

Considered only changing the view but the logic would have complicated their code.

Benefits

Visual cue of what files/precedence that are ignored.

Possible Drawbacks

Potential bugs/regressions not covered by current test suite, I guess.

@lee-dohm

This comment has been minimized.

Copy link
Member

lee-dohm commented Jan 4, 2018

I apologize for the long wait on this.

In taking a deeper look here, we would be interested in accepting this change if it had separate style for CVS ignored files and files that are ignored via core.ignoredNames. The reason for this is that it could be confusing to people if their ignored names are all of a sudden indistinguishable from the CVS ignored files and there's no way for them to restore the original behavior. So perhaps something like status-ignored-name for things that are ignored via core.ignoredNames?

What do you think?

@viddo

This comment has been minimized.

Copy link
Contributor Author

viddo commented Jan 4, 2018

No worries, I know you have a lot of PRs to review. :)

Yeah, I think your suggestions is sensible. Do you have any recommendation for the styling though?
It appears the current styles are defined in the themes and are intended for git alone (e.g. atom/one-dark-ui/styles/git.less which I'm using)., so I guess it would make more sense add a new style specific for tree-view, right? Also, the current status-ignored is using the @text-color-subtle from the official styleguide but there aren't that many other "subtle" color options to choose from. :/

@lee-dohm

This comment has been minimized.

Copy link
Member

lee-dohm commented Jan 4, 2018

Apologies for my incorrect terminology. I was suggesting that you create an additional class name like status-ignored-name and then don't style it at all by default. The theme authors (or individuals in their styles.less) would have to add it separately after that.

viddo added some commits Jan 6, 2018

Set unique status when ignored by name
Rather than (re-)use the git status
@viddo

This comment has been minimized.

Copy link
Contributor Author

viddo commented Jan 6, 2018

Ah ok, that makes sense. Merged with master to fix the conflicts and changed the status to be different if ignored by named as you suggestion. Looks OK?

BTW, looks like the test suite is malfunctioning on AppVeyor (same error on the latest PRs as well).

@daviwil

This comment has been minimized.

Copy link
Member

daviwil commented Feb 28, 2018

Thanks a lot @viddo!

@daviwil daviwil merged commit 4dbe842 into atom:master Feb 28, 2018

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

maxbrunsfeld added a commit that referenced this pull request Aug 20, 2018

Avoid using the project directory itself as an example for tests
The tree-view directory itself is ignored when these tests are run as
part of Atom's CI.

Refs #1048
Refs atom/atom#17088
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.