Skip to content

fix: added container wrapper component to handle styles#6476

Merged
vibhandikyash merged 16 commits into
releasefrom
FIX/2587-container-style-support
Sep 16, 2021
Merged

fix: added container wrapper component to handle styles#6476
vibhandikyash merged 16 commits into
releasefrom
FIX/2587-container-style-support

Conversation

@vibhandikyash

@vibhandikyash vibhandikyash commented Aug 9, 2021

Copy link
Copy Markdown
Contributor

Description

Added container wrapper for styling

Fixes #2587
Fixes #4419

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Tested locally.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Test coverage results 🧪

🟢 Total coverage has increased
// Code coverage diff between base branch:release and head branch: FIX/2587-container-style-support 
Status File % Stmts % Branch % Funcs % Lines
🟢 total 54.45 (0.02) 36.27 (-0.01) 33.59 (0.03) 55 (0.01)
✨ 🆕 app/client/src/components/designSystems/appsmith/WidgetStyleContainer.tsx 80.77 37.5 100 80.77
🔴 app/client/src/widgets/ContainerWidget/component/index.tsx 93.33 (0.23) 63.16 (-1.84) 100 (0) 93.33 (0.23)
🔴 app/client/src/widgets/FormWidget/widget/index.tsx 86.96 (-0.8) 52.63 (0) 91.67 (-0.64) 88.1 (-0.79)

@vibhandikyash
vibhandikyash requested a review from marks0351 August 9, 2021 09:31
@vibhandikyash vibhandikyash self-assigned this Aug 9, 2021
@vibhandikyash
vibhandikyash marked this pull request as ready for review August 10, 2021 11:13
@somangshu
somangshu removed their request for review August 10, 2021 12:07

@marks0351 marks0351 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Great job on adding the style features!!
Pls add a docs update PR for these changes.
when border is more than a certain value the widget overflows widget boundaries. it should not be the case.

@vibhandikyash

Copy link
Copy Markdown
Contributor Author

@marks0351 Here also updated Border styles for Text Widget for issue #4419

@marks0351 marks0351 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@vibhandikyash when a container radius is set very large the container goes away which is fine and is expected but the area below container becomes unaccessible because of the div overflow.
Also when border radius is set to a large number it does not work as expected with text widget. text widget starts to overflow.

somangshu
somangshu previously approved these changes Aug 27, 2021

@somangshu somangshu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approving to run tests

@somangshu

Copy link
Copy Markdown
Contributor

@vibhandikyash related test has failed, Please check

marks0351
marks0351 previously approved these changes Sep 1, 2021
@somangshu

Copy link
Copy Markdown
Contributor

Rerunning all jobs here, Draggable_PropertyPane_Widgets_spec.js failed because the property pane moved while widget is dragged down. Not related to the PR

@somangshu somangshu changed the title FIX #2587 : added container wrapper component to handle styles fix: added container wrapper component to handle styles Sep 1, 2021
@somangshu
somangshu removed their request for review September 1, 2021 06:50
@somangshu

Copy link
Copy Markdown
Contributor

Failed again, @vibhandikyash please check this locally and let me know

somangshu
somangshu previously approved these changes Sep 3, 2021

@somangshu somangshu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, @rishabhsaxena thanks for the help here.

Approving to run the tests

@somangshu

somangshu commented Sep 3, 2021

Copy link
Copy Markdown
Contributor

@marks0351 @vibhandikyash here is my feedback

Container widget:

  1. The old property background-colour should be above the new styles
  2. All the properties (styling related) should be under the style section
  3. Same for for the form widget
  4. The border is always cut from the side where there is a border radius Screenshot 2021-09-03 at 12 36 37 PM
  5. For older container / creating apps from DB the new properties are undefined which other wise is 0 when initially dragged and dropped

Text widget:

  1. OP requested for the margin property, which is not implemented (is this intended): Since the border can be used as the margin property, we should add this in the border width helper text.
  2. Other properties were not requested in the text widget, today placing the text widget in the container widget get you the attributes you need. So in my view, these new properties are not required in the text widget

@appsmithorg appsmithorg deleted a comment from YogeshJayaseelan Sep 3, 2021

@marks0351 marks0351 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@vibhandikyash the text widget and container still overflows when border width given is high.

marks0351
marks0351 previously approved these changes Sep 9, 2021

@NandanAnantharamu NandanAnantharamu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@YogeshJayaseelan

Copy link
Copy Markdown
Contributor

@vibhandikyash Verified the container style functionality and looks good to me

@vivekverma2312

Copy link
Copy Markdown

@vibhandikyash Working fine for text widget too.

@vibhandikyash
vibhandikyash merged commit aea5501 into release Sep 16, 2021
@vibhandikyash
vibhandikyash deleted the FIX/2587-container-style-support branch September 16, 2021 04:21
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.

[Feature] Text widget should have padding controls Container styles

7 participants