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

refactor #7

Merged
merged 33 commits into from Sep 30, 2020
Merged

refactor #7

merged 33 commits into from Sep 30, 2020

Conversation

ryangjchandler
Copy link
Collaborator

@ryangjchandler ryangjchandler commented Sep 1, 2020

Opening this up as a draft initially just so you guys can track the progress.

The basic idea is to migrate to microbundle for bundling the magic properties, write a decent test suite with Jest and move some bits around like utility functions and move to an Object.start() method instead of a generic function.

Note: These changes also require a repository name change to magic-helpers instead of alpine-magic-helpers. I think this is fine though since it's assumed that it is related to Alpine..

@ryangjchandler
Copy link
Collaborator Author

Microbundle supports multiple entry files through the source property inside of package.json, so whenever a new magic property is added, it would need to be added to that array.

Alternatively, we could invoke microbundle from inside a custom Node script that takes all of the files from that directory, but that seems a bit overkill at the moment.

@ryangjchandler
Copy link
Collaborator Author

I've written some tests for the $truncate magic method since it's the simplest one, @KevinBatdorf I think I've covered all of the important scenarios.

@KevinBatdorf
Copy link
Collaborator

I'm actually going to be pretty busy this week but I took a quick look and the refactoring and tests look good.

Just noticed something else I think we should implement. Doesn't feel right having punctuation at the end of a truncated string. Maybe remove that but give the option to enable it. Not for this PR obviously but just a thought.

.toEqual('Lorem ipsum dolor sit amet,…')

Another thought, maybe we set up a project board where we drop ideas and little tweaks like this?

@SimoTod
Copy link
Collaborator

SimoTod commented Sep 2, 2020

I like the idea of having a project board with upcoming tasks. If we trim out punctuation, I propose we also remove spaces (I find it 'Lorem ipsum dolor sit amet …' weird)

@ryangjchandler
Copy link
Collaborator Author

I like the idea of having a project board with upcoming tasks. If we trim out punctuation, I propose we also remove spaces (I find it 'Lorem ipsum dolor sit amet …' weird)

Yeah, I agree. I also had a weird case where , was being included in the trimmed string, so it wasn't really truncating based on words. I wonder if we can remove any trailing commas, full-stops too.

@KevinBatdorf
Copy link
Collaborator

Do you think we should use Component in place of AlpineComponentMagicMethod, etc?

Also, what do you think about adding a changelog file?

@ryangjchandler
Copy link
Collaborator Author

ryangjchandler commented Sep 8, 2020

Do you think we should use Component in place of AlpineComponentMagicMethod, etc?

Also, what do you think about adding a changelog file?

Yeah, we could use Component instead if you wanted. I'll try to get some time to finish this PR off later on, but I can update it to that yeah.

Changelog makes sense. I think we need to stick to a format though, perhaps https://keepachangelog.com?

@KevinBatdorf
Copy link
Collaborator

That format looks good. Maybe preface the component name too?

## [1.0.0] - 2017-06-20
### Added
- [$truncate] Adds option for ....
### Fixed
- [$parent] Adds fix for ...etc etc

@ryangjchandler
Copy link
Collaborator Author

That format looks good. Maybe preface the component name too?

## [1.0.0] - 2017-06-20
### Added
- [$truncate] Adds option for ....
### Fixed
- [$parent] Adds fix for ...etc etc

Yeah, I like the idea of that.

@SimoTod SimoTod force-pushed the chore/bundler branch 2 times, most recently from 8e94158 to a00dbe1 Compare September 17, 2020 11:15
@ryangjchandler
Copy link
Collaborator Author

@SimoTod feeling risky with all those force pushes are we ;)

HugoDF
HugoDF previously approved these changes Sep 26, 2020
@KevinBatdorf
Copy link
Collaborator

Added some additional tests for the edge cases that had recent bug fixes.

My build seems to have changed the spacing of the build files. Is that something we should fix in the editor config of somewhere?

@SimoTod
Copy link
Collaborator

SimoTod commented Sep 30, 2020

My build seems to have changed the spacing of the build files. Is that something we should fix in the editor config of somewhere?

It's correct. It should have 4 spaces now. I think I didn't rebuild dist after merging the version check PR.
We need to fix the conflict in component.js... again :(

@KevinBatdorf
Copy link
Collaborator

We need to fix the conflict in component.js... again :(

I'll do that now

@KevinBatdorf
Copy link
Collaborator

Looks like the other PR had target[key] != null instead of target[key] !== null so that was conflicting.

I think we're good to merge now if you agree. Will the GH action update the version numbers automatically after we tag?

Copy link
Collaborator

@SimoTod SimoTod left a comment

Choose a reason for hiding this comment

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

looks good to me

@SimoTod
Copy link
Collaborator

SimoTod commented Sep 30, 2020

Will the GH action update the version numbers automatically after we tag?

yeah, when you tag a new version, it should update package.json and push to npm

@KevinBatdorf
Copy link
Collaborator

@SimoTod Could it update the readme somehow? Otherwise we should probably update that here then before merging

@SimoTod
Copy link
Collaborator

SimoTod commented Sep 30, 2020

The cdn links? I'd like the github action being responsible for updating the readme. We can merge it master now, but I think we don't need to tag a new release until we add a new helper or we fix a bug. There won't be any benefit at the minute.

@KevinBatdorf KevinBatdorf merged commit 9f462fb into master Sep 30, 2020
Issues and Features automation moved this from In progress to Done Sep 30, 2020
@KevinBatdorf KevinBatdorf deleted the chore/bundler branch September 30, 2020 11:27
@KevinBatdorf
Copy link
Collaborator

Merged! Great job everyone. I think this puts the project in a much better place now.

@HugoDF
Copy link
Collaborator

HugoDF commented Sep 30, 2020

@SimoTod Could it update the readme somehow? Otherwise we should probably update that here then before merging

We can make this happen, a JS script that reads from the package.json and replaces versions in the README

@SimoTod
Copy link
Collaborator

SimoTod commented Sep 30, 2020

The github action already knows the current tag and it runs on unix, I was thinking to just use sed to replace those strings so we don't need an additional script

@HugoDF
Copy link
Collaborator

HugoDF commented Sep 30, 2020

Yeah that would work as well created an issue #21

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants