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

Vue rewrite #86

Merged
merged 13 commits into from
Jan 15, 2022
Merged

Vue rewrite #86

merged 13 commits into from
Jan 15, 2022

Conversation

miller-kevin
Copy link

@miller-kevin miller-kevin commented Jan 13, 2022

Here's the first pass at the vue rewrite. All 3 primary functionalities work. Wouldn't complain if anyone wanted to help test it. It's currently stationed in a subfolder because I'm sure you don't want to totally remove the jquery version.

As a heads up, there were some changes made for use in my org. They're strictly styling changes, but things like transparent backgrounds so the grey doesn't stick out like a sore thumb were the biggest. Those are included here because I think transparent backgrounds should be the default.

You'll also notice that it no longer respects data attributes or classes set on the root level. We can add props to make those more usable, but I don't think it's necessary.

Side note: I just realized I actually didn't work from the latest branch. I'll handle merging in animateOnView tomorrow

@miller-kevin miller-kevin mentioned this pull request Jan 13, 2022
@teobais
Copy link
Owner

teobais commented Jan 13, 2022

@miller-kevin thank you for this piece of work. Is it only my local environmental that gets a sh: vue-cli-service: command not found when I try to run it, or do you also have it on a fresh installation?

@miller-kevin
Copy link
Author

@teobais no, I've never seen that error. Looks like the Vue cli GitHub has some answers vuejs/vue-cli#2404

Also, if you're trying to run it, make sure you're running npm install first, then npm run serve in the directory

@teobais
Copy link
Owner

teobais commented Jan 13, 2022

@miller-kevin it seems there were a few old remnants in my local npm repo that were messing up with package-lock.json and brew. Went for a clean install, and it seems that the current setup works as expected.

Feel free to continue as planned with the animateOnView
Also, it would be best if the demo page showcased all the supported functionality (like here); that could serve as "manual regression" if you wish

@miller-kevin
Copy link
Author

miller-kevin commented Jan 13, 2022

@teobais Took another pass at this. Added animate on scroll and the demo page as you requested. Bunch of styling bug fixes as well

@teobais
Copy link
Owner

teobais commented Jan 14, 2022

@miller-kevin thanks for the update. Does that mean we are ready for review? If so, I'll do my best to have a look at in the weekend. Also, I see two GitHub accounts of yours here. Which one would you prefer to be mentioned in the contributors list?

@teobais
Copy link
Owner

teobais commented Jan 14, 2022

@miller-kevin another thing I neglected mentioning so far is the size variety: does the proposed solution still offer at least three pre-set sizes to choose from (small, default, big) as the jquery solution does?

@miller-kevin
Copy link
Author

@teobais if there are two accounts, I'm guessing it's because I changed my old username from whoisme555 to miller-kevin, so miller-kevin would be preferred.

I believe it's ready for review, yes.

As for size configuration, I can add that. I removed it because it's just changing font size, so I didn't see the value in having presets that the user will just override anyways. I'll leave that up to you if you want it added back

@teobais
Copy link
Owner

teobais commented Jan 15, 2022

@miller-kevin yes, please, let's keep it for the time-being.

@miller-kevin
Copy link
Author

@teobais Alright, additional sizes added

@teobais teobais merged commit a53bb2c into teobais:master Jan 15, 2022
@teobais
Copy link
Owner

teobais commented Jan 15, 2022

@miller-kevin thank you again for this contribution. Would you mind letting me know of the name of your company and how you use percircle? I'm creating an inventory of enterprise use cases of percircle. If not able to share here, my social DMs are open. Thank you again, highly appreciated.

@miller-kevin
Copy link
Author

@teobais I've used this at Accent Technologies. It's used in a part of the application meant to display sales lead quality and allow managers to get a display of a seller's performance at a glance. We initially had a jQuery solution and I've been slowly migrating us to Vue, so this change has been on my radar for a while anyways.

@teobais
Copy link
Owner

teobais commented Jan 17, 2022

@miller-kevin seems like there are few companies with the same name, could you please provide a link? Also, please let me know if your company would have any objection to being listed as a percircle user some time in the future

Another thing is that the language tag of the current repo seems to have changed to Vue, which shouldn't be the case, but I guess the heavy package-lock.json is to blame for :) . That being said, I consider transferring the vuejs port to a new repo, after making a few changes. Would you be interested in being listed a collaborator/maintainer there?

@miller-kevin
Copy link
Author

@teobais https://accent-technologies.com/ I don't know if they'd object. I assume they'd be fine with it.

A new repo makes sense for something like this. Would be more clear to users if they could run something like npm install percircle-vue instead of manually adding the file to their solution. I don't mind being listed as a collaborator/maintainer. I can contribute as time permits

teobais added a commit to teobais/percircle-vue that referenced this pull request Jan 18, 2022
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