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

#380 PR of Enhancing Docstrings & Highlighting Potential Issues #393

Merged
merged 6 commits into from
Nov 7, 2023

Conversation

ranggakd
Copy link
Contributor

@ranggakd ranggakd commented Nov 1, 2023

Issue #380 @pplonski

Primary Goal of this PR:

  • Address and add the missing docstrings of input widgets

Secondary Discoveries:

  • Some logical and testing issues that I believe warrant attention.

However, to maintain the focus of this PR on docstrings, I've summarized my findings in the table below (potentially another new issue) rather than making direct changes.

Widget Risk Concerns About Fix Integration
App - title: Originally defaults to ''. Empty string default might lead to confusion as it will use the notebook's filename. Changing default to "Title" improves UX. There's a potential need to make this a mandatory parameter since the empty default seems purposeless.
- description: Originally defaults to ''. An empty string means no displayed text. Changing default to "Description" encourages better explanation, even though some cases might warrant its absence.
- Consider making title a required parameter.
- The defaults for output, schedule, and notify need to be reviewed and tested. Potential uncertainties exist about their allowed options and behavior.
Button label: Originally defaults to ''. An empty string default results in a button with no visible text, leading to a poor UI/UX experience (appears as a bulky, nondescript line). Changing the default to "Button" improves the visual appearance and provides clarity to users. There's potential consideration to make this a mandatory parameter since the empty default seems to serve no purpose and lacks error handling or feedback for such cases. Consider making label a required parameter due to its impact on UI/UX. The original implementation lacks assertions, error handling, or feedback mechanisms for an empty string label.
Checkbox label: Originally defaults to ''. An empty string default results in no accompanying text next to the checkbox, which in this context is manifested as a toggle switch. This leads to a lack of context for users. The checkbox typically requires a label to provide context for its function. Changing the default to "Checkbox" provides clarity. There's significant merit in considering making this a mandatory parameter since the empty default seems to serve no purpose and lacks error handling or feedback for such cases. Recommend making label a required parameter to provide necessary context. The original implementation lacks assertions, error handling, or feedback mechanisms for an empty string label.
File label: Originally defaults to 'File upload'. Even when an empty string is passed, the widget functions, but its context is lost. A label is integral to providing clarity and understanding about the purpose and function of the file widget. A scenario where no label is required for this widget is hard to envisage. Recommend making label a required parameter to ensure necessary context for the user. The current implementation lacks assertions, error handling, or feedback mechanisms for situations with an empty string label.
Note text: Originally defaults to an empty string. When the default is empty, it provides no content in the markdown-formatted note, which is counterintuitive as the sole purpose of the widget is to display a note. Changed the default to "Note" to better signify its markdown nature. Recommend making text a required parameter. The current setup lacks assertions, error handling, or feedback for an empty string, especially given that text is the only key parameter.
Numeric label: Originally defaults to an empty string. When the default is empty, it provides no context or text above the numeric widget, reducing its usability and clarity. The label provides necessary context to the user about the purpose or usage of the numeric input. Changed the default to "Numeric" for a clearer representation. Recommend making label a required parameter. The current design lacks assertions, error handling, or feedback for an empty string, especially given the label's importance for context.
Select - label: Originally defaults to an empty string. When left empty, it provides no context or text above the dropdown, making it unclear for users. Changed the default for label to "Select" for a clearer representation. The label provides necessary context to the user about the dropdown's purpose.
- choices: By default, it's an empty list. An empty choices list with a None value for the value parameter makes the widget non-functional and lacks purpose.
- url_key: The URL encoding needs a review, especially for spaces in options.
- Recommend making label a required parameter due to its importance in context provision.
- Similarly, the choices parameter should also be required since an empty list defeats the widget's purpose.
- The URL encoding behavior of url_key needs refinement to ensure special characters like spaces are appropriately encoded and showed for correct URL semantics.
MultiSelect - label: Originally defaults to an empty string. Without a label, there's a lack of clarity and context for users interacting with the dropdown. Changed the default for label to "MultiSelect" for better clarity.
- choices: Defaults to an empty list. When combined with an empty list value, the widget is non-functional and lacks its core purpose.
- url_key: The URL encoding needs a review, especially for spaces in options.
- It's recommended to make label a required parameter due to its vital role in providing context.
- The choices parameter also should be required to ensure the widget's functionality is always preserved.
- Improved the logic for value initialization for better alignment with multi-select behavior.
- The URL encoding behavior of url_key should be refined to properly encode special characters.
Slider label: Originally defaults to an empty string, which results in no contextual text displayed above the widget. This absence can be confusing and detract from user experience, as there's no clear indication of what the slider pertains to. Changed the default for label to "Slider" to provide a clearer indication of the widget's purpose. It's recommended to make label a required parameter due to its importance in context provision. The absence of validation for this default setting can lead to unclear interfaces and diminish the user experience.
Text - label: Originally defaults to an empty string, leading to no contextual text displayed above the widget. The absence of this text can cause confusion and hampers user experience, as the user won't have a clear understanding of the widget's function. Modified the default for label to "Text" to provide a clearer indication of the widget's role.
- url_key: The URL encoding needs a review, especially for spaces in options.
- The recommendation is to make label a mandatory parameter given its pivotal role in conveying context. The lack of validation for the original default can lead to a less intuitive user experience.
- The URL encoding behavior of url_key should be refined to properly encode special characters.
Range label: Originally defaults to an empty string, causing no contextual text to be displayed above the widget. This lack of context can be confusing for users and deteriorate the user experience since the purpose of the widget might not be immediately apparent. Updated the default for label to "Range" to provide a clearer indication of the widget's function. Given the importance of the label in offering context, it is suggested to make this parameter mandatory. The absence of validation or feedback for an empty string in the original design can result in a confusing UI/UX.

The concerns about default empty strings; can be ambiguous. In user interfaces, having descriptive placeholders can guide the user to understand what's expected of them. Making a parameter mandatory can be beneficial if it's crucial for the functioning or user experience of the widget. If it isn't mandatory, then having robust error handling and feedback mechanisms in place can mitigate potential issues.

Copy link
Contributor

@pplonski pplonski left a comment

Choose a reason for hiding this comment

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

Thank you! Great job!

@pplonski pplonski merged commit 5fa3eed into mljar:main Nov 7, 2023
@pplonski
Copy link
Contributor

pplonski commented Nov 7, 2023

Hi @ranggakd,

Great update. I like default labels values that you set. For App the schedule and notify need to be implemented. Notebook scheduling is not ready yet.

Do you have twitter and linkedin so we can thank you in our social profiles?

@ranggakd
Copy link
Contributor Author

ranggakd commented Nov 7, 2023

Hello again @pplonski,

Thank you for your kind words! I’m thrilled to hear that you found my contributions valuable. It was a pleasure working on this project, and I’m glad I could help improve it. Your feedback is very encouraging, and it motivates me to continue contributing.

I appreciate the offer to be recognized on your social profiles. Here are my links:

My single source of links


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.

2 participants