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

Improve Assembly icon #990

Merged
merged 4 commits into from
Jan 13, 2023
Merged

Improve Assembly icon #990

merged 4 commits into from
Jan 13, 2023

Conversation

ignamartinoli
Copy link
Contributor

[what]
Adds consistence with other low level languages icons like C/C++.

[note]
Closes #857.

@all-contributors please add @ignamartinoli code

Signed-off-by: igna_martinoli ignamartinoli@protonmail.com

Description

Closes this issue and improves the existent icon by making it more consistent with other low level languages icons.

Requirements / Checklist

What does this Pull Request (PR) do?

Improve the Assembly icon by making it consistent with other low level languages like C/C++

How should this be manually tested?

I really don't know.

Any background context you can provide?

Discussion here

What are the relevant tickets (if any)?

None.

Screenshots (if appropriate or helpful)

image

@Finii
Copy link
Collaborator

Finii commented Nov 8, 2022

Thanks for the design an the PR.

I would not replace the existing 'asm', as it is part of Seti UI. We should keep that as we keep the other, simpler 'C' and 'C++' icons. All other nerd font's own svgs are now called *_nf.svg.
What is missing is an entry in the icons.tsv. (See README.md in src/svgs/.)

Do you want to amend the PR, or shall I?

@Finii Finii self-requested a review November 8, 2022 05:33
Copy link
Collaborator

@Finii Finii left a comment

Choose a reason for hiding this comment

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

Please do not replace the Seti icon but rather add an additional one (with filename ending in _nf.svg. Also this needs a new entry in the icons 'database'.

@ignamartinoli
Copy link
Contributor Author

I'm taking a shot at this. I plan on contributing more icons in the future so I want to be able to do it correctly 😉

@Finii
Copy link
Collaborator

Finii commented Nov 8, 2022

👍 I hope it's not too many as the available slots are diminishing ;-)

@ignamartinoli
Copy link
Contributor Author

Alright, I reverted the icon change and added the icon as asm_nf.svg.
I also changed icons.tsv, it looks like this

...
59		i_custom_asm				asm_nf.svg
# new
058		i_seti_kotlin
...

Reading through src/svg/README.md, should I run generate-original-source.py myself or is it going to run automatically at Push?

src/svgs/icons.tsv Outdated Show resolved Hide resolved
@Finii
Copy link
Collaborator

Finii commented Nov 8, 2022

should I run generate-original-source.py myself

Not as part of the PR, but you can to see if your changes work.
That would have warned about the fact offset 59 is handed out twice... lets see

$ ./generate-original-source.py

[Nerd Fonts]  Glyph collection font generator 2.3.0-RC

Conflicting filename for 58677, ignoring apple.svg
Read glyph data successfully with 196 entries (19 aliases)
Generating original-source.otf with 177 glyphs
Generating GlyphInfo i_seti.sh
Finished

optimize-original-source.sh and generate-original-source.py are automatically run, the CI should have added commits to your PR automatically. Ah, because it is in your fork, it did not run, I assume. It will be run on pulling.

CI process: https://github.com/ryanoasis/nerd-fonts/blob/master/.github/workflows/packsvgs.yml

Edit: Add CI workflow file link

Finii and others added 3 commits January 13, 2023 09:02
[why]
The 'new' in the middle can be confusing. What is the meaning?
Also the offsets are easier to graps if they are all equally wide (i.e.
have leading padding zeros).

See #990 (comment)

Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
[what]
Adds consistance with other low level languages icons like C/C++.

Reverted the change on `asm.svg`, added `asm_nf.svg`, modified `icons.tsv`

[note]
Closes #857.
Closes [990#pullrequestreview-1171459043](#990 (review))

Signed-off-by: igna_martinoli <ignamartinoli@protonmail.com>

[Note by Fini: Squashed two commits.]

Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
[why]
The new asm icon replaces the apple icon.
I believe this has been entered as offset by mistake.

[how]
Add new icon to bottom.

Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
@Finii
Copy link
Collaborator

Finii commented Jan 13, 2023

Icon itself looks good:

image

[why]
When we add new icons to the original-source the generator updates the
cheat sheet list. But that is never committed to the repo.

Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
@Finii Finii merged commit db39c25 into ryanoasis:master Jan 13, 2023
Finii added a commit that referenced this pull request Jan 13, 2023
[why]
The 'new' in the middle can be confusing. What is the meaning?
Also the offsets are easier to graps if they are all equally wide (i.e.
have leading padding zeros).

See #990 (comment)

Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
Finii pushed a commit that referenced this pull request Jan 13, 2023
[what]
Adds consistance with other low level languages icons like C/C++.

Reverted the change on `asm.svg`, added `asm_nf.svg`, modified `icons.tsv`

[note]
Closes #857.
Closes [990#pullrequestreview-1171459043](#990 (review))

Signed-off-by: igna_martinoli <ignamartinoli@protonmail.com>

[Note by Fini: Squashed two commits.]

Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
@Finii
Copy link
Collaborator

Finii commented Jan 13, 2023

CI is broken on Github side.
The CI needs to run for packsvgs.yml :-(

Keeping to try, maybe it will be repaired in an hour or so.

@Finii
Copy link
Collaborator

Finii commented Jan 13, 2023

Strange, why could the CI do this, while we could not...:

image

Is this a bug with dry-run? 🤔

Edit: No. ... very strange

LNKLEO pushed a commit to LNKLEO/Nerd that referenced this pull request Nov 24, 2023
[why]
The 'new' in the middle can be confusing. What is the meaning?
Also the offsets are easier to graps if they are all equally wide (i.e.
have leading padding zeros).

See ryanoasis#990 (comment)

Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
LNKLEO pushed a commit to LNKLEO/Nerd that referenced this pull request Nov 24, 2023
[what]
Adds consistance with other low level languages icons like C/C++.

Reverted the change on `asm.svg`, added `asm_nf.svg`, modified `icons.tsv`

[note]
Closes ryanoasis#857.
Closes [990#pullrequestreview-1171459043](ryanoasis#990 (review))

Signed-off-by: igna_martinoli <ignamartinoli@protonmail.com>

[Note by Fini: Squashed two commits.]

Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
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.

Add Netwide Assembler (NASM) icon
2 participants