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

[17.0][IMP] web_timeline: Update vis-timeline-graph2d lib: 7.7.0 -> 7.7.3 #2972

Merged
merged 1 commit into from
Oct 30, 2024

Conversation

carlos-lopez-tecnativa
Copy link
Contributor

@OCA-git-bot
Copy link
Contributor

Hi @tarteo,
some modules you are maintaining are being modified, check this out!

Copy link
Member

@chienandalu chienandalu left a comment

Choose a reason for hiding this comment

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

isn't the diff much larger than needed? I don't see that many changes in the related release notes 🤔 for example, looks like css stuff is just moved from one place to another in the css file...

@@ -889,6 +889,21 @@
border-style: solid;
border-radius: 4px;
}
.vis .overlay {
Copy link
Member

Choose a reason for hiding this comment

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

Why increasing the diff moving the styles ?

@pedrobaeza
Copy link
Member

Yes, and the library JS seems also very different. Maybe one version is minified and the other not?

@carlos-lopez-tecnativa
Copy link
Contributor Author

carlos-lopez-tecnativa commented Oct 23, 2024

I only copied the files generated by executing the npm run build command. The result is similar to previous updates, as seen in the commit.

94323da

@pedrobaeza
Copy link
Member

pedrobaeza commented Oct 23, 2024

Well, it's the double in fact (48k additions instead of 24k). Not sure if there can be any command line for changing somehow the result.

@carlos-lopez-tecnativa
Copy link
Contributor Author

Well, it's the double in fact. Not sure if there can be any command line for changing somehow the result.

I'm not sure, but if @chienandalu or @CarlosRoca13 can help me with the command used in the previous case, that would be great. The difference is mainly in the indentation.

image

@chienandalu
Copy link
Member

The difference is mainly in the indentation.

That's not hard to deal with. Just select the whole block and indent it to the right :)

@carlos-lopez-tecnativa
Copy link
Contributor Author

The difference is mainly in the indentation.

That's not hard to deal with. Just select the whole block and indent it to the right :)

Yes, but I meant that the current spacing is 2
image

and now it's 4. I think the new one is better, although it shows more differences. In the next updates, the differences should be smaller.
image

@chienandalu
Copy link
Member

Yes, but I meant that the current spacing is 2

Ah, ok. In that case I think is ok :) I just saw your first capture where all the code was pulled to the left

@pedrobaeza
Copy link
Member

Can't you configure a different indentation or open it and change it for minimizing the diff?

@carlos-lopez-tecnativa
Copy link
Contributor Author

Can't you configure a different indentation or open it and change it for minimizing the diff?

I took the file directly from https://unpkg.com/[email protected]/standalone/umd/vis-timeline-graph2d.js
If I manually adapt the code, the result will be very different again in the next update.

@pedrobaeza
Copy link
Member

Are we sure the next time it will be indented the same way?

@pedrobaeza pedrobaeza added this to the 17.0 milestone Oct 30, 2024
Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

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

As the diff seems to not be avoidable, let's merge it:

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 17.0-ocabot-merge-pr-2972-by-pedrobaeza-bump-patch, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit e802b70 into OCA:17.0 Oct 30, 2024
7 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 6b62ffb. Thanks a lot for contributing to OCA. ❤️

@pedrobaeza pedrobaeza deleted the 17.0-web_timeline_update_lib branch October 30, 2024 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants