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

Modernize the project by adding native Typescript types. Making a class #20

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

andresgutgon
Copy link

What?

TopBar can be imported as an ESM project. And added the container option because I saw it as an open issue

I also moved the tooling for minimizing to Rollup which allows minification with terser but also Typescript transpilation and different export module formats

Open question

Do you have an npm link. I think it would be great to publish as an NPM package.

@andresgutgon andresgutgon force-pushed the feature/esm-typescript-container branch 3 times, most recently from 56c5502 to f8c6c9a Compare November 1, 2024 13:59
TopBar that can be imported as an ESM project. And added container
option because I saw in an open issue.

Also moved the tooling for minimizing to Rollup which allow minification
with terser but aslo Typescript transpilation and differen export module
formats
@andresgutgon andresgutgon force-pushed the feature/esm-typescript-container branch from f8c6c9a to fccad96 Compare November 1, 2024 14:02
],
"version": "3.0.0",
"version": "3.0.1",
Copy link
Author

Choose a reason for hiding this comment

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

I think a patch version is fine here. If I did it ok previous behaviour should still work. I tried documentation page and it keeps working as a script.

The difference is that now people can import as ESM module

// ./src/app.ts
import { Topbar } from 'topbar' // ? I don't think is jet published

declare global {
  interface Window {
    topbar: Topbar
  }
}

window.topbar = new Topbar({
  barColors: {
    0: 'rgba(255, 153, 153, .9)',
    '.17': 'rgba(255, 204, 153, .9)',
    '.34': 'rgba(255, 255, 153, .9)',
    '.51': 'rgba(153, 255, 153, .9)',
    '.68': 'rgba(153, 204, 255, .9)',
    '.85': 'rgba(204, 153, 255, .9)',
    '1.0': 'rgba(204, 153, 255, .9)',
  },
  shadowColor: 'rgba(0, 0, 0, .1)',
})

Choose a reason for hiding this comment

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

I'm just watching this project but here's my two cents.

IMO, a minor version bump is more appropriate for this PR. Bumping the patch version is reserved just for patches (bug fixes). Since this PR isn't fixing bugs but rather adding new features, a minor bump would be the better way to go.

Copy link
Author

Choose a reason for hiding this comment

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

Agree, true

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.

2 participants