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.

Sign up for GitHub

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

Jump to bottom

[CLOSED] Themes in Brackets core #6857

Open
7 of 8 tasks
core-ai-bot opened this issue Aug 30, 2021 · 30 comments
Open
7 of 8 tasks

[CLOSED] Themes in Brackets core #6857

core-ai-bot opened this issue Aug 30, 2021 · 30 comments

Comments

@core-ai-bot
Copy link
Member

Issue by MiguelCastillo
Thursday Apr 24, 2014 at 01:08 GMT
Originally opened as adobe/brackets#7616

Initial commit. A few items remain to be done, but themes are now fully functional as a core component so you can start testing this out.

https://github.com/adobe/brackets/wiki/Themes

  • ThemeManager goes into core.
  • ExtensionLoader has special logic for themes extensions that don’t have JS
  • Settings pared down to just your “General” screen
  • All of the other loading/scanning code can go away
  • Use jQuery promises, sorry :*(
  • Settings screen should not use Knockout
  • Use new prefs API
  • Unit tests for theme loading and the customizations (scrollbars, etc.)

MiguelCastillo included the following code: https://github.com/adobe/brackets/pull/7616/commits
@core-ai-bot
Copy link
Member Author

Comment by MarcelGerber
Thursday Apr 24, 2014 at 01:39 GMT

All used strings should be localized.

And btw, Travis failed (because of missing JSLint comments).

@core-ai-bot
Copy link
Member Author

Comment by MiguelCastillo
Thursday Apr 24, 2014 at 01:48 GMT

Yeah, there are a few items in my list for code cleanup and integrate with what brackets already has. I think that by tomorrow or friday, code will be more inline with what we all expect :)

@core-ai-bot
Copy link
Member Author

Comment by MiguelCastillo
Thursday Apr 24, 2014 at 01:49 GMT

Love how quickly you guys are providing feedback! Its awesome

@core-ai-bot
Copy link
Member Author

Comment by dangoor
Thursday Apr 24, 2014 at 01:59 GMT

For some added context for those tuning in to this pull request right now: I asked Miguel if he could convert his extension into a pull request against core as something that we could begin discussing and also so that it was possible for it to load themes as extensions, which is not possible in an extension today. I added (REVIEW ONLY) because there's definitely more coming before this feature lands.

@core-ai-bot
Copy link
Member Author

Comment by MiguelCastillo
Saturday Apr 26, 2014 at 13:07 GMT

Alright, I think we are at a point where feedback would be great. Themes code is fully integrated into Brackets.

I created a theme that is already in the Extension Registry. The theme has to have a main.js or the registry wouldn't allow me to load it. So, we might need to adjust something there. This is the git link for the theme also
https://github.com/MiguelCastillo/Brackets-VisualStudioTheme

I am going to evaluate inline editor and mixin support. adobe/brackets#4850

@core-ai-bot
Copy link
Member Author

Comment by MarcelGerber
Sunday Apr 27, 2014 at 18:52 GMT

https://github.com/adobe/brackets/wiki/Brackets-Coding-Conventions

@core-ai-bot
Copy link
Member Author

Comment by MiguelCastillo
Sunday Apr 27, 2014 at 20:39 GMT

So, pretty much all the feedback has been stylistic/convention. I will read the coding conventions guide; thanks@SAPlayer.

This is all fantastic, but I am really looking more for feedback regarding the code itself. I am touching stuff in ExtensionLoader.js and ExtensionManager.js. And to be honest, opinions asides, it's far more important that all that stuff functions correctly and I am not introducing regressions; I can easily remove the white spaces later and what not later.

@core-ai-bot
Copy link
Member Author

Comment by MiguelCastillo
Sunday Apr 27, 2014 at 20:41 GMT

@TomMalbran Yeah, I have to move that stuff to the less files... I have been a bit hesitant to completely remove stuff from themes and spread it around in brackets. Emotional attachment :) I will certainly move the styles and html files over to their corresponding folders.

@core-ai-bot
Copy link
Member Author

Comment by TomMalbran
Sunday Apr 27, 2014 at 20:46 GMT

I wanted to test this PR, but it isn't working for me. I am on the. Brackets fails while loading and doesn't even show any errors in the console, so I don't know where it fails. It loads the menus, the UI and then it crashes. Neither the project or the extensions are loaded, so somewhere before that it breaks. Not sure if the same happens to you.

One other thing. You are merging from master, but it might get easier to merge from a branch in your repo.

@core-ai-bot
Copy link
Member Author

Comment by MiguelCastillo
Sunday Apr 27, 2014 at 21:02 GMT

@TomMalbran Yeah it has been working for me just fine... I have been doing my daily development on it.

Hmmm, I am really looking for better way to manage this merging process. I have a PR from a clone. What other way do you think would be better? Please help on this front :D

@core-ai-bot
Copy link
Member Author

Comment by TomMalbran
Sunday Apr 27, 2014 at 21:10 GMT

I am still a bit a noob in Git, so I am not sure how you could move your changes in master to a branch. The How to Hack on Brackets wiki has some basic info on working in branches and a clone of the brackets repo: https://github.com/adobe/brackets/wiki/How-to-Hack-on-Brackets

@core-ai-bot
Copy link
Member Author

Comment by MarcelGerber
Sunday Apr 27, 2014 at 21:27 GMT

@TomMalbran It's not important as I don't expect Miguel wants to create another PR while this one is still running. It would be better for the next PR, but nothing important.

The only thing that wouldn't be bad would be to update to the current master.@MiguelCastillo you can do that like this:

[if not already] git remote add upstream https://github.com/adobe/brackets
git fetch upstream
git merge upstream/master
git submodule update --init

It could be that there are merge conflicts (e.g. zaggino's Check for extension updates).
Well, you need to fork and clone the repo unless you are a commiter.

@core-ai-bot
Copy link
Member Author

Comment by MiguelCastillo
Sunday Apr 27, 2014 at 21:28 GMT

@SAPlayer Running grunt does not catch these spaces between parenthesis you are talking about. Is that something we should adjust somewhere?

@core-ai-bot
Copy link
Member Author

Comment by MiguelCastillo
Sunday Apr 27, 2014 at 21:28 GMT

@SAPlayer Thanks for those git steps dude!

@core-ai-bot
Copy link
Member Author

Comment by MarcelGerber
Sunday Apr 27, 2014 at 21:30 GMT

I don't even know what grunt does to be honest :) You have to ask someone else.

Ah btw, before you push the changes, better test everything locally. It's easier to revert there.

@core-ai-bot
Copy link
Member Author

Comment by TomMalbran
Sunday Apr 27, 2014 at 21:32 GMT

If there is a way to make this into a branch it might be better, since you don't need to keep updating the master branch.

Grunt uses JSHint to lint (instead of JSLint used in Brackets) which I think that isn't showing those errors. But if you open any file in Brackets with JSLint enabled, you will see all the spascing errors.

@core-ai-bot
Copy link
Member Author

Comment by MarcelGerber
Sunday Apr 27, 2014 at 21:42 GMT

@TomMalbran The easiest way is just git checkout -b new_branch_name, but the branch used by the PR can't be changed later on. The only way would be to create a completely new PR, based on the new branch.

@core-ai-bot
Copy link
Member Author

Comment by TomMalbran
Sunday Apr 27, 2014 at 21:44 GMT

Is not really a problem to create a new PR (we can point to the old PR if required), my point is mostly about moving the changes made in master to that branch.

@core-ai-bot
Copy link
Member Author

Comment by MiguelCastillo
Sunday Apr 27, 2014 at 21:45 GMT

@TomMalbran Yikes! I was relying on grunt as that's what was mentioned I should use. Seems like a total disconnect to have your build depend on jshint and local edits on jslint. :(

@core-ai-bot
Copy link
Member Author

Comment by MiguelCastillo
Sunday Apr 27, 2014 at 21:48 GMT

Let me rebase as@SAPlayer has suggested... Let's see if that works.

@core-ai-bot
Copy link
Member Author

Comment by TomMalbran
Sunday Apr 27, 2014 at 21:50 GMT

It is weird. I am not sure why they had JSHint as the linter in Grunt and not JSLint (maybe there wasn't an npm package for JSLint at that point, or there was a problem with it). But JSHint from grunt is configured to work like JSLint, but it is not exactly the same. We will eventually move to use JSHint in Brackets too, so that will fix this discrepancy.

For now, check your files against JSLint in Brackets and fix the issues. We have to manually check that when reviewing PR.

@core-ai-bot
Copy link
Member Author

Comment by MiguelCastillo
Sunday Apr 27, 2014 at 21:52 GMT

@SAPlayer You are awesome. It worked! Its very easy to impress me when it comes down to git :D

@core-ai-bot
Copy link
Member Author

Comment by MiguelCastillo
Sunday Apr 27, 2014 at 21:53 GMT

@TomMalbran that makes me really sad its not in grunt... But on the bright side, when Brackets moves to JSHint, we will be in a better place :)

@core-ai-bot
Copy link
Member Author

Comment by MiguelCastillo
Sunday Apr 27, 2014 at 21:54 GMT

@TomMalbran also, I just merged all the changes in brackets master into my branch. So, the PR should have all latest and greatest.

@core-ai-bot
Copy link
Member Author

Comment by MiguelCastillo
Sunday Apr 27, 2014 at 22:46 GMT

@TomMalbran is the latest code working for ya?

@core-ai-bot
Copy link
Member Author

Comment by TomMalbran
Sunday Apr 27, 2014 at 22:59 GMT

No. It still doesn't work :(

@core-ai-bot
Copy link
Member Author

Comment by MiguelCastillo
Sunday Apr 27, 2014 at 23:19 GMT

Odd! I have it running right now. I merged latest code from master, so I guess that was not it. You can't get a stacktrace I am assuming?

@core-ai-bot
Copy link
Member Author

Comment by dangoor
Monday Apr 28, 2014 at 12:41 GMT

This past weekend was pretty full for me... sorry to have missed all of the comments!

It is possible to move your work to a branch and then reset your master to a point back before you started working on the themes integration... that would likely do crazy things for anyone who may have cloned your code, but that's not too bad. But, you would have to start a new pull request. If you're not itching to get something else going on your master branch, it seems like you're best off continuing this PR for a bit longer until things have settled enough to create a new PR (probably with a flattened history).

I'll take a preliminary look through the code now.

@core-ai-bot
Copy link
Member Author

Comment by dangoor
Tuesday Apr 29, 2014 at 13:14 GMT

This is shaping up nicely. One more general comment: we don't tend to use as many small files. I think you can likely combine some into larger logical units.

@core-ai-bot
Copy link
Member Author

Comment by MiguelCastillo
Wednesday Apr 30, 2014 at 12:59 GMT

Sorry folks, I have been a bit absent... Work stuff has been keeping me busy. @dangoor That's great feedback. I will digest that stuff and hopefully I can integrate all that stuff today. Thanks for taking a look at this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@core-ai-bot