Remove codicon.css, document and explain future usage #312
Conversation
There was a problem hiding this comment.
I'm ok with the changes (but see comments below). @ndoschek @planger @tortmayr please have a look as well.
This brings up a more general question to me regarding the original introduction of the codicons dependency in #249: Shouldn't this be a dependency of the concrete application or downstream library rather than the core package? The sprotty package itself does not need this dependency, only the applications that want to include such icons do. A lot of users of the sprotty package do not need codicons. Would it be feasible to drop the dependency and leave it to the responsibility of the package users to bundle the icons if they need them?
packages/sprotty/build/tsconfig.json
Outdated
| @@ -0,0 +1,16 @@ | |||
| { | |||
There was a problem hiding this comment.
The build script is not very large. I'd propose to write it as a .js file with // @ts-check annotation so there's no need for a separate tsc call and config file.
There was a problem hiding this comment.
Yes, that is a good idea, plus the "debug" functionality (generateAlernateLoadingFunction and storage in separate files) could be removed, too.
There was a problem hiding this comment.
Transformed it to JavaScript and removed everything unnecessary.
| const ttfData = `data:font/ttf;base64,${dataTtf}`; | ||
| const alteredCssData = cssData.replace('"./codicon.ttf?0e5b0adf625a37fbcd638d31f0fe72aa"', ttfData); | ||
|
|
||
| fs.writeFileSync('./css/generated/codicon.css', alteredCssData); |
There was a problem hiding this comment.
This is a redistribution of the codicon content and so we need to attribute that correctly according to their license:
There was a problem hiding this comment.
Yes, thank you, I didn't check that, yet.
There was a problem hiding this comment.
The generated CSS contains links to both license and the original Microsoft Copyright header. There must be some mentioning of codicon usage in the README. Should I add that?
In general, I agree this probably should be a dependency of the downstream project. This way the consuming project/library could is also in complete control of the codicon version that should be used.
That's currently not completely true. It seems like default implementation of the command palette is currently using codicons. |
|
@tortmayr ping |
kaisalmen
force-pushed
the
codicon-transform
branch
from
24fa0d0 to
622fd7a
Compare
|
I have squashed and force pushed the branch on current main and performed a bit of clean-up. We could use this workaround/solution for now until we find a potential better solution. It would help prevent for example this problem: #311 (comment) |
There was a problem hiding this comment.
I think we can go right to the cleaner solution instead of introducing a workaround.
My proposal:
- Remove the dependency in package.json
- Remove this import: https://github.com/eclipse/sprotty/blob/d9b542a9974d7db03742010917ca64dc9d0eed76/packages/sprotty/src/features/command-palette/di.config.ts#L16
- Document in CHANGELOG.md that those who are using the command palette need to import the codicons CSS in their app.
|
Ok, in addition I will apply add the codicon css to the classdiagram example. Then it it is not only documented, but people can see how it is actually done (we can even reference that from the CHANGELOG). |
|
@spoenemann Sorry for the late reply. The proposed changes look good to me 👍🏼 |
kaisalmen
changed the title
kaisalmen
force-pushed
the
codicon-transform
branch
from
622fd7a to
6563649
Compare
|
@spoenemann The rework is done. This is now ready for review. I fought with the webpack config, beacuse codicon.ttf was not loaded. In the end, removing the extra config removed the problem. webpack 5 (introduced in between initial addition of command palette) changed asset handling. |
This is the proposed solution for #311