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

Svg axis and text opacity #1223

Merged
merged 2 commits into from
Apr 3, 2021
Merged

Svg axis and text opacity #1223

merged 2 commits into from
Apr 3, 2021

Conversation

rocky
Copy link
Member

@rocky rocky commented Apr 3, 2021

@mmatera with a change to the text opacity, I think we get most of the axis ugliness addressed. Also there is one slight adjustment to how svg <text ..../> is set up.

Let me know what you think.

More generally also allow InsetBox opacity to be specified.
Split <text> parameters in template into text_pos_opts, text_style_opts, and css_style.
@rocky rocky requested a review from mmatera April 3, 2021 02:16
@mmatera
Copy link
Contributor

mmatera commented Apr 3, 2021

OK, I like this, but actually, to improve the axis labels the best would be to implement the Align property, in a way that the labels of the y-axis be aligned to the right, and the labels of the x axis be aligned to the top.

@mmatera mmatera merged commit 7484267 into fix_to_svg Apr 3, 2021
@rocky rocky deleted the svg-axis-and-text-opacity branch April 3, 2021 02:53
@rocky
Copy link
Member Author

rocky commented Apr 3, 2021

OK, I like this, but actually, to improve the axis labels the best would be to implement the Align property, in a way that the labels of the y-axis be aligned to the right, and the labels of the x axis be aligned to the top.

Absolutely, but let me give the broader context around why I did this and did this now.

@mmatera At this point, I'd like to have a release in about 24hours and I am trying to understand what will make the cut and what won't.

I was borderline on this, until I spent some time with the code to glean how much of a mess we would have to undo in the future, how ugly it looks, and the benefit. With the merge in (thank you), I think this should go in. I would like some of the commits (like the one where you are just removing print statements that in my opinion shouldn't have been commited in the first place) squashed.

The PRs labled "needs work" I would like deferred

These appear to as broken:

These I would like deferred to another release

Since there is already a lot of changes here including some big ones, if there is anything that is not ready and can be deferred I would prefer they get deferred. I would rather have two smaller releases than one big one.

Your thoughts?

@mmatera
Copy link
Contributor

mmatera commented Apr 3, 2021

@mmatera At this point, I'd like to have a release in about 24hours and I am trying to understand what will make the cut and what won't.

Completely agree. I think with the transparency it actually looks nicer...

I was borderline on this, until I spent some time with the code to glean how much of a mess we would have to undo in the future, how ugly it looks, and the benefit. With the merge in (thank you), I think this should go in. I would like some of the commits (like the one where you are just removing print statements that in my opinion shouldn't have been commited in the first place) squashed.

I guess I did it the squash...

Regarding the rest:

#1216
#1213
#1212
#1217
are ready. #1215 is ready, in the sense that the test is ready. If you can also ship the package, go ahead, but still, I do not have any subset of the package working, just tests that eventually will detect when the package does something interesting.

I would also keep #1209, because it actually improves a lot the support of the kernel in comparisons. I guess it could be a little bit better implemented, but not a lot.

#1199 collect some of the previous PRs, so you do not need to merge it.

@mmatera
Copy link
Contributor

mmatera commented Apr 3, 2021

Regarding #1220, this is something that we should fix as soon as possible after we finish with the release since many other things depend on the ability to handle derivatives.

@rocky
Copy link
Member Author

rocky commented Apr 3, 2021

@mmatera At this point, I'd like to have a release in about 24hours and I am trying to understand what will make the cut and what won't.

Completely agree. I think with the transparency it actually looks nicer...

I was borderline on this, until I spent some time with the code to glean how much of a mess we would have to undo in the future, how ugly it looks, and the benefit. With the merge in (thank you), I think this should go in. I would like some of the commits (like the one where you are just removing print statements that in my opinion shouldn't have been commited in the first place) squashed.

I guess I did it the squash...

Regarding the rest:

#1216
#1213
#1212
#1217
are ready.

Merged now.

#1215 is ready, in the sense that the test is ready. If you can also ship the package, go ahead, but still, I do not have any subset of the package working, just tests that eventually will detect when the package does something interesting.

I just looked at in more detail. #1215 not ready.

I would also keep #1209, because it actually improves a lot the support of the kernel in comparisons. I guess it could be a little bit better implemented, but not a lot.

I am not going to have time to go over #1209. From what I saw before it needs a refactor to make it more scalable, and at the same time we can increase the number of passing test cases.

So let's defer and after the release, let's work on this.

I will add a section in the release notes what's will be coming soon.

#1199 collect some of the previous PRs, so you do not need to merge it.

It has been closed.

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.

None yet

2 participants