-
Notifications
You must be signed in to change notification settings - Fork 332
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
docs(calling): improve samples UI. add back getDevice #3092
docs(calling): improve samples UI. add back getDevice #3092
Conversation
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.
could you add a screenshot like before and after code changes format ?
docs/samples/calling/style.css
Outdated
} | ||
|
||
.docs { | ||
overflow: auto; | ||
height: 98vh; | ||
max-width: 1400px; |
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 we convert this px
value to ``rem value here ?
replace to
max-width: 87.5rem
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.
Done
select { | ||
padding: 0 0.5rem; | ||
height: 1.75rem; | ||
border-radius: 4px; |
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 we use rem value here 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.
We should not change border radius if the root font-size changes.
} | ||
|
||
section { | ||
background-color: #fff; | ||
padding: 1rem; | ||
margin-bottom: 2rem; | ||
margin-bottom: 1.5rem; | ||
border-radius: 8px; |
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.
Here 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.
Same as above
6abb36e
to
b4387db
Compare
b4387db
to
f409589
Compare
f409589
to
4a21c48
Compare
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.
Found few issues while testing, please fix those and code should be good for approval
cd9c3d9
to
78e9ba9
Compare
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.
Code Changes LGTM
78e9ba9
to
95dcd93
Compare
COMPLETES #< SPARK-458857 >
This pull request addresses
by making the following changes
Before
After
Change Type
The following scenarios where tested
< ENUMERATE TESTS PERFORMED, WHETHER MANUAL OR AUTOMATED >
I certified that
I have read and followed contributing guidelines
I discussed changes with code owners prior to submitting this pull request
I have not skipped any automated checks
All existing and new tests passed
I have updated the documentation accordingly
Make sure to have followed the contributing guidelines before submitting.