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

Refactoring and some suggestions for improvements. #49

Closed
ronaldosc opened this issue Apr 14, 2023 · 9 comments
Closed

Refactoring and some suggestions for improvements. #49

ronaldosc opened this issue Apr 14, 2023 · 9 comments
Labels
wontfix This will not be worked on

Comments

@ronaldosc
Copy link

ronaldosc commented Apr 14, 2023

  • I'll bring modifications for start a PR refac of the exportations in the src/index.ts, because a lot of lines repeat.
  • a proposal for new name of "GameController" because it more generic and I encountered "Joystick" to fit well in this case and compatible with the visual icon.
  • at the core\src\icons.ts I think it more comprehensible the legacy icons being presented in the list as such as this example:
 name: "file-dashed",
    pascal_name: "FileDashed",
    legacy: {
      alias_name: "file-dotted",
      pascal_name: "FileDotted",
      changed_in: 2.0,
    },

And the prop changed_in a propose for register knowing when the legacy started (the future we don't know).


And some cases that the icon is published the first time, it's not necessary to fill prop updated_in cause it occurred at the same time this new implementation.

  { ...........,
    published_in: 1.4,
    updated_in: 1.4,
  },

would be only for this case

  { ..........,
    published_in: 1.4,
  },

What do you say about that ? Can assign me for this work ? I almost finished it.

@ronaldosc
Copy link
Author

@rektdeckard , what you think? We can discuss better if needed. Or with another contributors.

@rektdeckard
Copy link
Member

rektdeckard commented Apr 24, 2023

Hey @ronaldosc , thanks for you interest in making some improvements. To address your comments in order:

I'll bring modifications for start a PR refac of the exportations in the src/index.ts, because a lot of lines repeat.

Nothing is repeated and there is no need to touch this. It is a barrel file that we generate, and should not be edited directly. Yes, there is a more concise export syntax, but we bundle and minify the library anyway. Changes here do not mean real bytes saved.

a proposal for new name of "GameController" because it more generic and I encountered "Joystick" to fit well in this case and compatible with the visual icon.

We try not to rename icons except in the case the current name causes real confusion. I don't think it is true here, so let's leave it as-is. Plus, we have another more joystick-like game controller icon planned for the next release that more accurately matches that name.

at the core\src\icons.ts I think it more comprehensible the legacy icons being presented in the list as such as this example:

I understand wanting to add changed_in here, but the data backing this file actually lives an a Google Table we use to track our Dev process, and that info isn't present in the table and would need to be baclfilled. Can you explain your use-case? If you are at this point in code, you have the library and assets already. Legacy names are still supported as aliases, so you will never have access to an icon name that is not valid, either now or in the past. Also, again, this is generated code, so not the place to put this change anyway.

@rektdeckard rektdeckard added the wontfix This will not be worked on label Apr 24, 2023
@ronaldosc
Copy link
Author

ronaldosc commented Apr 24, 2023

The changed_in is only for icons that was changed when it occcurs. It's for reference and documentation.

And the pupose of changing the core\src\icons.ts some properties is for more comprehension of what it is. In the way now some of then are repeated, take a look by yourself.

@ronaldosc
Copy link
Author

about the two fields I've explained above:
`published_in: 1.4,
updated_in: 1.4,´

@ronaldosc
Copy link
Author

The refact in src/index.ts have a lot of repeated words.

But if you stay in a rigid position for a refactoring, I understand.

@rektdeckard
Copy link
Member

The source file is transpiled to much smaller code, which is the only code that is consumed when you install the library. The transpiled code is all that matters in terms of bytes, and the syntax we use in the source file does not change what the bundler will emit.

@ronaldosc
Copy link
Author

For comparison, with refact the index with .d.ts and map is about 43.4 KB, and the actual version emits 214.0 KB,

@rektdeckard
Copy link
Member

Can you show me what you are proposing, via an actual PR? I can't talk about this in the abstract. .d.ts and sourcemap files should not be of any concern; they exist only on developers machines and aren't bundled in a compiled app. And if you're using a CDN, you won't be fetching this file.

@rektdeckard
Copy link
Member

My guess is you mean turning exports like:

import { default as o } from "./icons/AddressBook.es.js";
import { default as t } from "./icons/AirTrafficControl.es.js";
export { 
  o as AddressBook,
  t as AirTrafficControl
};

into

export { default as AddressBook } from "./icons/AddressBook.es.js";
export { default as AirTrafficControl } from "./icons/AirTrafficControl.es.js";

Because I can't see any terser way to export everything as named, while still providing default exports in icon source files themselves. This really doesn't save many bytes at all, maybe 6 bytes per icon, plus another 10 or so bytes total for the single export statement.

@rektdeckard rektdeckard closed this as not planned Won't fix, can't repro, duplicate, stale May 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

2 participants