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

WIP: y scrolling code block, select block added, debug filter block e… #3

Merged
merged 25 commits into from
Oct 13, 2023

Conversation

DivadNojnarg
Copy link
Collaborator

Can't create DRAFT...

@DivadNojnarg DivadNojnarg added documentation Improvements or additions to documentation enhancement New feature or request labels Sep 13, 2023
@DivadNojnarg DivadNojnarg self-assigned this Sep 13, 2023
@nbenn
Copy link
Collaborator

nbenn commented Sep 22, 2023

@DivadNojnarg I'm sorry, I tried to fix some issues, but probably broke some of your work on select/plot blocks in doing so. One of the goals was to be able to specify "dependencies" between fields (and data). I am trying out something using quoted expressions which then can be evaluated in the context of data and other field values, e.g.:

https://github.com/cynkra/blockr/blob/16837a030da3118a76820e30a9c8b8200cee9480/R/block.R#L177

Happy to discuss!

@DivadNojnarg
Copy link
Collaborator Author

@nbenn . I just merged your changes in this MR. Going to check the select and plot blocks.

Screenshot 2023-09-26 at 11 22 15

@codecov
Copy link

codecov bot commented Sep 26, 2023

Codecov Report

Merging #3 (a8f5e09) into main (9ffeeca) will decrease coverage by 1.61%.
The diff coverage is 59.43%.

❗ Current head a8f5e09 differs from pull request most recent head 9ee4712. Consider uploading reports for the commit 9ee4712 to get more accurate results

@@            Coverage Diff             @@
##             main       #3      +/-   ##
==========================================
- Coverage   55.78%   54.17%   -1.61%     
==========================================
  Files           7        7              
  Lines         294      371      +77     
==========================================
+ Hits          164      201      +37     
- Misses        130      170      +40     
Files Coverage Δ
R/field.R 88.37% <ø> (ø)
R/stack.R 75.00% <100.00%> (ø)
R/utils.R 65.30% <100.00%> (+0.72%) ⬆️
R/server.R 0.00% <0.00%> (ø)
R/ui.R 78.37% <83.63%> (+10.08%) ⬆️
R/block.R 50.83% <29.54%> (-17.92%) ⬇️

R/block.R Outdated
new_filter_block <- function(data, columns = colnames(data)[1L],
values = character(), ...) {

sub_fields <- function(data, columns) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@nbenn : do we expect module developers to write this?

Copy link
Collaborator

@nbenn nbenn Sep 28, 2023

Choose a reason for hiding this comment

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

It depends what the module author wants to achieve.

If you have static fields, it's as simple as

https://github.com/blockr-org/blockr/blob/3bd11a2a033bc9dc3c2c90b721f41401eb4c78de/R/block.R#L139-L148

If you have a simple dependency on the data you can get away with something like

https://github.com/blockr-org/blockr/blob/9ffeecaec752391f181fbbfbd4c5d948b19f8133/R/block.R#L177-L182

(where in the newest iteration, you'd do col_choices <- function(data) colnames(data) instead of cols <- quote(colnames(.(data))) but that's a minor detail)

Now if you want to enable arbitrary "functional" dependencies between fields, the logic for what kind of actions changes the block state in what way, then yes, this logic has to be represented somehow. Tbh, I personally don't find

https://github.com/blockr-org/blockr/blob/54ae5802b4986165fb9fdc91a86562b727add47f/R/block.R#L160-L187

to be that unwieldy. This simple specifies what kind of "sub fields" are added for what kind of columns. Something like this could even be offered as exported utility function (as long as the user is happy with our type-mapping decisions).

As for

https://github.com/blockr-org/blockr/blob/54ae5802b4986165fb9fdc91a86562b727add47f/R/block.R#L190-L215

I agree, this is a bit harder to appreciate. The problem I see here is tension between ending up with an "idiomatic" expression (like using infix `|`() vs. prefix between()) and compactly/readably expressing the underlying logic. Over time, we might be able to offer some utilities to make this easier. But for now I was more trying to explore what is possible and how something like that could be done.

@codecov
Copy link

codecov bot commented Sep 28, 2023

The author of this PR, DivadNojnarg, is not an activated member of this organization on Codecov.
Please activate this user on Codecov to display this PR comment.
Coverage data is still being uploaded to Codecov.io for purposes of overall coverage calculations.
Please don't hesitate to email us at [email protected] with any questions.

@DivadNojnarg DivadNojnarg merged commit cdb56e0 into main Oct 13, 2023
5 checks passed
@DivadNojnarg DivadNojnarg deleted the select-block branch October 13, 2023 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants