fix: added text and changed margins #12923
Conversation
AAfghahi
changed the title
Codecov Report
@@ Coverage Diff @@
## master #12923 +/- ##
===========================================
- Coverage 69.79% 52.97% -16.83%
===========================================
Files 1026 480 -546
Lines 48865 17315 -31550
Branches 5261 4485 -776
===========================================
- Hits 34106 9172 -24934
+ Misses 14634 8143 -6491
+ Partials 125 0 -125
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| @@ -220,7 +220,7 @@ | |||
| ); | |||
|
|
|||
| // ** `.form-group` margin | |||
| @form-group-margin-bottom: 16px; | |||
| @form-group-margin-bottom: 8px; | |||
There was a problem hiding this comment.
we're going to be removing the .less styling, and also so that we don't change things too globally, you can instead add some styling for this selector with the emotion styles in DatasourceEditor.jsx under const DatasourceContainer = styled.div
There was a problem hiding this comment.
Hello, I was also uncomfortable changing this, since it is a global change. In the ticket, Stephen mentioned that he wanted the change for all form groups. So I thought that it might be for everything. Relooking at it, he might have meant for all of the form groups on this page however.
Will go back and change it using emotion styles instead. It will be a good opportunity to work with emotion.
There was a problem hiding this comment.
Sorry for taking so long, Beto was going through the backend architecture. I just made this change.
|
I think the margins above the titles should stay at what they were before (16px?) and just the margin between the input field and the help text should be 8px, but @Steejay can confirm! |
|
@AAfghahi sorry about the miscommunication. shouldve been more clear here. i was referring to the form groups on this page. |
|
Hello,
No need to apologize! I was reading it too literally.
Anyway, I needed to target the help span rather than the group. The newest
push accounts for it-
[image: Screen Shot 2021-02-03 at 9.15.33 PM.png]
[image: Screen Shot 2021-02-03 at 9.08.35 PM.png]
…On Wed, Feb 3, 2021 at 7:27 PM Stephen Jordan ***@***.***> wrote:
@AAfghahi <https://github.com/AAfghahi> sorry about the miscommunication.
shouldve been more clear here. i was referring to the form groups on this
page.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#12923 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ALVKTWG3PQVJTGUZJISJOUDS5HSWBANCNFSM4XBLSHEQ>
.
--
Arash Afghahi
|
| @@ -51,13 +51,17 @@ import { FeatureFlag, isFeatureEnabled } from 'src/featureFlags'; | |||
|
|
|||
| const DatasourceContainer = styled.div` | |||
| .change-warning { | |||
| margin: 16px 10px 0; | |||
| margin: 16px 10px 0x; | |||
There was a problem hiding this comment.
No, and I noticed that there was one test that I failed, and maybe this is why. Thank you for noticing this.
|
@AAfghahi can you repaste the new screenshots? looks like they're not showing up. thanks! |
|
Sorry about the screenshots, I replied via email, and that does not add them.
|
|
|
||
| .help-block { | ||
| margin-top: 8px; | ||
| } |
There was a problem hiding this comment.
when I render this, I still see the margin-bottom of the form-group, so it's still making a total of 16px.
This is a bit tricky actually. I read this as
form-group should always have a margin bottom of 8px
when there are two siblings (form-group followed by a form-group), then the second one should have a margin-top of 16px.
I think if you go that way, you won't need to add anything additional to the help-block.
lmk if that helps
There was a problem hiding this comment.
Yeah that helps a lot, it really cleared up my thinking about it. I also ended up reading more about how margin works. I ended up trying something else, because nothing I was reading was helping me find a way of implementing your solution.
I ended up targeting the specific form group that is above each help block (form-group-md). I changed the margin-bottom of these to be 8px. However, in order to make sure that 'Extra' and 'Owners' section had a help block with a margin of 8px, I also added a margin-top to the help-block.
Originally I thought that a margin-bottom of 8 and a margin-top of 8 on two elements would make 16px, but they don't actually add the way that I thought they might. It simple goes to the highest margin value. Which is cool.
It looks like this now:
The margin between form groups is 16px:
The margin between help-block and form-group is 8px:
There was a problem hiding this comment.
This look great. One other thing I was hinting at was sibling selectors: when there are two siblings (form-group followed by a form-group), then the second one should have a margin-top of 16px. So that would be .form-group + .form-group but it looks like you made it work this way, too.
* master: fix: UI toast typo (apache#13026) feat(db engines): add support for Opendistro Elasticsearch (AWS ES) (apache#12602) fix(build): black failing on master, add to required checks (apache#13039) fix: time filter db migration optimization (apache#13015) fix: untranslated text content of Dashboard page (apache#13024) fix(ci): remove signature requirements for commits to master (apache#13034) fix: add alerts and report to default config (apache#12999) docs(changelog): add entries for 1.0.1 (apache#12981) ci: skip cypress if no code changes (apache#12982) chore: add cypress required checks for branch protection (apache#12970) Refresh dashboard list after bulk delete (apache#12945) Updates storybook to version 6.1.17 (apache#13014) feat: Save datapanel state in local storage (apache#12996) fix: added text and changed margins (apache#12923) chore: Swap Slack Url 2 more places (apache#13004)
mistercrunch
added
🏷️ bot
SUMMARY
Also changed the form-group to have margin 8px for all form groups.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TEST PLAN
ADDITIONAL INFORMATION