plotly.rs icon indicating copy to clipboard operation
plotly.rs copied to clipboard
verified publisher icon plotly

Add `Table` trace.

Open baiguoname opened this issue 2 years ago • 7 comments

https://github.com/igiagkiozis/plotly/issues/148 This issue mentioned the trace.

baiguoname avatar Sep 23 '23 12:09 baiguoname

Really great to see a Table Trace in the pipeline. Would also love to see a Pie Chart Trace if possible. Would love to contribute this, but don't have the requisite skill to do it yet.

Nnamdi-sys avatar Oct 01 '23 06:10 Nnamdi-sys

Really great to see a Table Trace in the pipeline. Would also love to see a Pie Chart Trace if possible. Would love to contribute this, but don't have the requisite skill to do it yet.

This is also first time I make a pr in plotly, after this pr is merged, I would like to make a similar Pie Chart pr.

baiguoname avatar Oct 01 '23 09:10 baiguoname

Really great to see a Table Trace in the pipeline. Would also love to see a Pie Chart Trace if possible. Would love to contribute this, but don't have the requisite skill to do it yet.

This is also first time I make a pr in plotly, after this pr is merged, I would like to make a similar Pie Chart pr.

Hi @igiagkiozis, would be great to have this pull request approved in your next release. I tried it out and it works quite well. I also find it to be a very useful feature.

Nnamdi-sys avatar Oct 08 '23 09:10 Nnamdi-sys

@baiguoname I'm glad someone started work on this! I do think it would make more sense if Table::new() takes a header and a cells struct, unless I'm missing something (I am a rust noob). I think in your code currently there is no way to change the header or cells style without creating a new instance of a header or cell with the same values you used in Table::new() and altering the header field in the struct with Table.header(). It makes more sense to me to create the cell or header beforehand with the values and styles you want and pass it into the table when you're happy with it. I hope that makes sense. I've made a fork of your repo and am happy to submit a pr to it :) Lmk what you think. Heres the link to my fork: https://github.com/kylemello/plotly

kylemello avatar Jan 01 '24 20:01 kylemello

@baiguoname I'm glad someone started work on this! I do think it would make more sense if Table::new() takes a header and a cells struct, unless I'm missing something (I am a rust noob). I think in your code currently there is no way to change the header or cells style without creating a new instance of a header or cell with the same values you used in Table::new() and altering the header field in the struct with Table.header(). It makes more sense to me to create the cell or header beforehand with the values and styles you want and pass it into the table when you're happy with it. I hope that makes sense. I've made a fork of your repo and am happy to submit a pr to it :) Lmk what you think. Heres the link to my fork: https://github.com/kylemello/plotly

Thanks very much, I think it's great. Excuse me, I'm still new to github, what should I do next? Should I close this commit, and you pull a new pr to this crate?

baiguoname avatar Jan 04 '24 08:01 baiguoname

Thanks very much, I think it's great. Excuse me, I'm still new to github, what should I do next? Should I close this commit, and you pull a new pr to this crate?

Sorry for the late reply, I swear github never notifies me of anything. No, I don't think so... I think me submitting a pr onto your branch will do just fine. I think once you merge it you'll be set. I haven't done a pull request on a pull request on GitHub yet so I'm just guessing here lol. Just opened it, here's the link: https://github.com/baiguoname/plotly/pull/1

kylemello avatar Jan 09 '24 07:01 kylemello

@baiguoname hi, is this feature still able to be merged? Really looking forward to see it.

jm18919 avatar May 15 '24 01:05 jm18919

Is it ok for me to take over this PR and make it mergable? @baiguoname @andrei-ng

jm18919 avatar Jun 13 '24 00:06 jm18919

Is it ok for me to take over this PR and make it mergable? @baiguoname @andrei-ng

Sorry for late reply. Yes, it's ok for me.

baiguoname avatar Jun 13 '24 01:06 baiguoname

Is it ok for me to take over this PR and make it mergable? @baiguoname @andrei-ng

Sorry for late reply. Yes, it's ok for me.

Thx. How do I take over it? I am not familiar with Github's PR. It looks like I just need to resolve merge conflict.

jm18919 avatar Jun 13 '24 01:06 jm18919

Is it ok for me to take over this PR and make it mergable? @baiguoname @andrei-ng

Sorry for late reply. Yes, it's ok for me.

Thx. How do I take over it? I am not familiar with Github's PR. It looks like I just need to resolve merge conflict.

Please do take it over :) .

My typical worflow is to add the other fork as another remote to my local clone, fetch the branch, check it out, do changes and push it back to the fork e.g.,

git remote add baiguoname [email protected]:baiguoname/plotly.git
git fetch baiguoname
git checkout -b table-pr baiguoname/master

... do changes

git push -u  table-pr HEAD:master

There might be other workflows outhere...

andrei-ng avatar Jun 13 '24 10:06 andrei-ng

I am not so sure how to link the conflict-resolve PR to this one....

jm18919 avatar Jun 17 '24 03:06 jm18919

@baiguoname can you merge my branch into your PR branch?

jm18919 avatar Jun 17 '24 04:06 jm18919

@baiguoname can you merge my branch into your PR branch?

Yes, of course. I have added a review to your pr . Is there anything else I need to do?

baiguoname avatar Jun 17 '24 09:06 baiguoname

@baiguoname I just update my branch to fix some CI issues. Can you approve it?

jm18919 avatar Jun 18 '24 08:06 jm18919

@baiguoname and @naijim , should we close this PR in favour of https://github.com/plotly/plotly.rs/pull/207 ?

andrei-ng avatar Jun 18 '24 11:06 andrei-ng

@baiguoname and @naijim , should we close this PR in favour of #207 ?

Yes, I think so.

baiguoname avatar Jun 18 '24 11:06 baiguoname