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

Support enum input type in launch form #178

Merged
merged 1 commit into from
Aug 4, 2021

Conversation

Pianist038801
Copy link
Contributor

@Pianist038801 Pianist038801 commented Jul 26, 2021

Signed-off-by: Pianist038801 steven@union.ai

TL;DR

Add enum type input in launch form

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

Tracking Issue

flyteorg/flyte#1217

Follow-up issue

NA

Signed-off-by: Pianist038801 <steven@union.ai>
@welcome
Copy link

welcome bot commented Jul 26, 2021

Thank you for opening this pull request! 🙌
These tips will help get your PR across the finish line: - Most of the repos have a PR template; if not, fill it out to the best of your knowledge. - Sign off your commits (Reference: DCO Guide).

@codecov
Copy link

codecov bot commented Jul 26, 2021

Codecov Report

❗ No coverage uploaded for pull request base (master@91695f8). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #178   +/-   ##
=========================================
  Coverage          ?   70.24%           
=========================================
  Files             ?      340           
  Lines             ?     6761           
  Branches          ?     1132           
=========================================
  Hits              ?     4749           
  Misses            ?     2012           
  Partials          ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 91695f8...6525e6c. Read the comment docs.

Copy link
Contributor

@jsonporter jsonporter left a comment

Choose a reason for hiding this comment

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

Left one comment (NIT) but once addressed lgtm.

import * as React from 'react';
import { DatetimeInput } from './DatetimeInput';
import { makeStringChangeHandler, makeSwitchChangeHandler } from './handlers';
import { InputProps, InputType } from './types';
import { UnsupportedInput } from './UnsupportedInput';
import { getLaunchInputId } from './utils';

const useStyles = makeStyles(theme => ({
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: not a blocker but do we need makeStyles here; couldn't we just send styles as a generic object? (is there a specific reason for useStyle?)

eg,

const formStyle = {
   minWidth: 100%; 
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we define a style that way, we should use style props of React component.
But I found that we currently use className prop for styling.

That's why I went with such solution, just tried to replicate how it's already implemented.

Copy link
Contributor

@jsonporter jsonporter left a comment

Choose a reason for hiding this comment

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

Lgtm

@jsonporter jsonporter merged commit 847461f into master Aug 4, 2021
@welcome
Copy link

welcome bot commented Aug 4, 2021

Congrats on merging your first pull request! 🎉

@flyte-bot
Copy link
Collaborator

🎉 This PR is included in version 0.22.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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.

3 participants