-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Refactor upload component and add styled upload component #3035
Conversation
""" | ||
# Set default props. | ||
props.setdefault("border", "1px dashed black") | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The commented styles break a previous form render test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably fix the test then, go ahead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests have been fixed. and I added some for the styled case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added extra information to the PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you make it work in dark mode too?
You could use var(--accent-12)
instead of black for the color of the border.
I'll work on that as well |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Will do a more thorough review later this week and try to get this in the next release
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we have a separate class StyledUpload
, we need to ensure that Upload.is_used
gets set, regardless of which flavor of upload is actually used, so that the App mounts the upload endpoints
I thought that calling the super() class method would set the variable, is there any need to set the is_used variable again? |
I'm curious, I don't understand how the react component works as I haven't used React before, but If we decide to do |
In that case it has nothing to do with React, it's purely a python situation, if you try Test with the following example : class Foo:
is_used = False
@classmethod
def create(cls):
cls.is_used = True
return cls()
class Bar:
is_used = False
@classmethod
def create(cls):
cls.is_used = True
return super()
Bar.create()
print(Foo.is_used, Bar.is_used) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for adding!
Description
These changes add default styling to the upload component - It follows the same pattern as the form component, where the root method call accesses the unstyled component.
Some default styles were commented so as not to break a previous test in the rendering of forms (With permission, I'd like to refactor the test).
Closes #2967
All Submissions:
Type of change
New Feature Submission:
Changes To Core Features: