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

Remove jquery from project #239

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Remove jquery from project #239

wants to merge 6 commits into from

Conversation

wishdasher
Copy link
Collaborator

I just commented out the code in ExpansionReqs because I couldn't figure out what the intended functionality was. If someone could look at that, I would really appreciate it.

@npfoss npfoss requested a review from georgiashay May 3, 2019 06:41
@georgiashay
Copy link
Collaborator

The code you commented out is to scroll to the top of the open req layer when you open or close a req

@wishdasher
Copy link
Collaborator Author

Hmm, I see what you mean, but my personal expected behavior is also that the expansion wouldn't cause things to jump around and simply expand and collapse below the click target.

@georgiashay
Copy link
Collaborator

It’s possible things have changed since the card has gotten bigger, but when I implemented that it was fairly annoying to close things and have to scroll up or down every time to be able to continue viewing it

@wishdasher
Copy link
Collaborator Author

I totally misunderstood what you meant. I thought you were talking about the audit tree.

However, the info card doesn't seem to scroll on prod right now when I do this either?

@georgiashay
Copy link
Collaborator

Just tested on prod and it worked for me. Are you looking at classes like 10.26 that have expandable requirements?

@wishdasher
Copy link
Collaborator Author

Okay, I finally figured out what you were talking about. I wasn't aware we had this.

@wishdasher
Copy link
Collaborator Author

All right, I re-implemented it in Javascript.

@npfoss npfoss mentioned this pull request May 4, 2019
Copy link
Collaborator

@georgiashay georgiashay left a comment

Choose a reason for hiding this comment

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

Not sure if it's possible to add the nice animations back in for scroll top in the class info card without jquery, but I liked how they looked before. Also, the road hash changing function was removed for some reason so that should be added back.

@@ -236,7 +235,7 @@ export default {
},
clickRelatedSubject: function (subject) {
this.$emit('push-stack', subject.id);
$('#cardBody').animate({ scrollTop: 0 });
document.getElementById('cardBody').scrollTop = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This no longer animates the scroll nicely. Not sure if there is a way to animate this without jquery? The scroll-behavior:smooth doesn't seem to be having the same effect.

Copy link
Member

Choose a reason for hiding this comment

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

There are two ways we can do this: figure out the CSS animation or drop in a replacement for jQuery that only handles animations. I can't comment on the former since I'm atrociously awful at CSS animation, but I have used Velocity.js as a replacement for jQuery before. It only contains animation libraries and should be smaller than jQuery, while also having more responsive animations in comparison.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think vuetify also has transitions that could be helpful here

@@ -113,13 +126,14 @@ export default {
this.open = false;
let scrollPoint;
if (!this.doubleScroller) {
scrollPoint = $('#' + $.escapeSelector(this.reqID));
scrollPoint = document.getElementById(escape(this.reqID));
Copy link
Collaborator

Choose a reason for hiding this comment

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

This scrolling also does not seem to be animated anymore.

borders.css({ top: 0, left: scrollWidth - 1 + scrollPosition });
});

$(window).on('hashchange', function () {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why was this removed?

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.

3 participants