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

Add markdown widget for interactive app forms #3767

Merged
merged 3 commits into from
Feb 3, 2025

Conversation

robinkar
Copy link
Contributor

Allows inserting arbitrary Markdown into the app forms through the form.yml files.
The largest use case for us, which I've also seen others request on Discourse (e.g. here and here), would be use this to create headings/sections in the app form.

For example, the following form YML:

attributes:
  slurm_partition:
    label: "Slurm Partition"
    widget: select
    options:
      - interactive
  cores:
    label: "Memory (GB)"
    widget: number_field
    value: 2
  header_resources:
    widget: markdown
    value: |
      ---
      ## Resources
  header_settings:
    widget: markdown
    value: |
      ---
      ## Settings

form:
  - slurm_partition
  - header_resources
  - cores
  - memory
  - header_settings
  - bc_email_on_started

would produce the following form:
image

This is not fully ready for merging (e.g. missing tests), but I'm submitting this to check whether this could be a potential solution/approach to this feature. Potentially fixes #1806.
I added serialize? for SmartAttributes since it would make sense to not store the value of this in any user_defined_context.json or saved settings file (bc_saved_settings), but .select(&:serialize?) should probably be refactored into a function if this approach is fine.

@johrstrom
Copy link
Contributor

I'm not sure about this implementation. Given that we have to .select(&:serialize?) everywhere and can't just pull the attributes tells me there's something amiss here. IDK it just adds complexity to the downstream consumers who have to always remember to filter these out.

That said - I get why you did it. We can't just add another property like header because bootstrap_form gem actually generates the entry, and we can't pass a block to it to add extra elements.

@robinkar
Copy link
Contributor Author

robinkar commented Sep 2, 2024

I made an attempt at improving it somewhat, although the core idea remains the same.
It seems like the distinction between the different attributes need to be made somewhere, and already exists to a certain extent with fixed attributes being handled differently, but the new logic here (i.e. serialize?) is now more limited to SessionContext.
The way I see it is that there are attributes that affect the execution of the job and are visible in the form (serializeable and visible, user-configurable), attributes that affect the execution of the job, but are not visible (fixed (non-serializable) and invisible, not user-configurable), and attributes that do not affect the execution of the job, but are visible (non-serializable and visible, introduced with this PR).
Downstream consumers would need to select the correct attributes depending on purpose, i.e. BatchConnect::App.attributes (all attributes), vs SessionContext.attributes/each (serializable only) vs SessionContext.each_form_attribute (visible attributes for form). A larger refactor would probably make these distinctions more clear, but I am not sure at the moment what that would look like.
Suggestions on how to improve this are much appreciated.

@johrstrom
Copy link
Contributor

johrstrom commented Sep 17, 2024

Suggestions on how to improve this are much appreciated.

It seems to me like we need catch the output here and maybe prepend it.

Something like this in create_widget. As you elude to, there are UI specific stuf, like label, and so header can be similar to that. Where we render the form field, but then prepend the header if it exists.

rendered =    case widget
              when 'select'
                form.select(attrib.id, attrib.select_choices(hide_excludable: hide_excludable), field_options, attrib.html_options)
              when 'resolution_field'
              # ...
              end

# may not even need the if block because nil won't render anything anyhow.
if attrib.header.nil?
  rendered
else
   header =  OodAppkit.markdown.render(attrib.header.to_s).html_safe
   "#{header}#{rendered}"
end

@johrstrom johrstrom self-requested a review January 27, 2025 18:39
Copy link
Contributor

@johrstrom johrstrom left a comment

Choose a reason for hiding this comment

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

Looks like it's been some time since you opened this. Is this still on-going? It seems I left a comment, but I haven't seen any updates. Are you waiting on me for something?

@robinkar robinkar closed this Jan 28, 2025
@robinkar robinkar force-pushed the app-form-markdown-widget branch from e90533b to 76ecb11 Compare January 28, 2025 10:53
@robinkar robinkar reopened this Jan 28, 2025
@robinkar
Copy link
Contributor Author

Looks like it's been some time since you opened this. Is this still on-going? It seems I left a comment, but I haven't seen any updates. Are you waiting on me for something?

Sorry for stalling this, some other tasks came up and I kept postponing this.
I just pushed a version with what you suggested, which should work well enough for us.

Only thing that might need more consideration would be how it interacts with the dynamic JS, e.g. data-hide-*. Right now, the header is always visible, which is fine in our case at least. In theory it could be useful to hide the header as well if all the form elements under that category/header are hidden, but in our case I don't see it ever being hidden.

Allows providing a header for elements in Markdown format that is
rendered before the element.
@robinkar robinkar force-pushed the app-form-markdown-widget branch from 36d9181 to 1b0ad78 Compare January 28, 2025 11:18
@johrstrom johrstrom self-requested a review January 28, 2025 15:24
@robinkar robinkar force-pushed the app-form-markdown-widget branch from 8b8ef6b to d18b9b6 Compare January 29, 2025 07:57
@johrstrom johrstrom self-requested a review February 3, 2025 17:04
Copy link
Contributor

@johrstrom johrstrom left a comment

Choose a reason for hiding this comment

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

Thanks! I'll add a test around sanitize shortly while it's on my mind.

@johrstrom johrstrom merged commit 2921466 into OSC:master Feb 3, 2025
22 checks passed
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.

Grouping form variables with headers
3 participants