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

Menubar material #1070

Closed
wants to merge 9 commits into from
Closed

Menubar material #1070

wants to merge 9 commits into from

Conversation

goyal-Dushi
Copy link

Addressing issue #1059.
Designed the Editor UI using material and CSS

editMenuUI.mp4

@vercel
Copy link

vercel bot commented Jul 3, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/smaranjitghose/doc2pen/4PJ8WN3zTjHyrD5K9yeQYcTsMykw
✅ Preview: https://doc2pen-git-fork-goyal-dushi-menubar-material-smaranjitghose.vercel.app

Copy link
Collaborator

@AbhipsaGuru1012 AbhipsaGuru1012 left a comment

Choose a reason for hiding this comment

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

@goyal-Dushi there are some changes that I would recommend

  1. The image that you have used for line spacing is slightly bigger than the other images that you have used. Please try to make it of the same height as the other images.

  2. Also the input field where you are showing the line-spacing value, can you expand it a bit to fit all the characters. I have attached an image below that shows the current display.
    doc2pen

@smaranjitghose smaranjitghose added LGMSOC21 Let's Grow More Summer of Code 2021 PR: reviewed-changes-requested and removed PR: unreviewed labels Jul 5, 2021
@goyal-Dushi
Copy link
Author

Hey @AbhipsaGuru1012, I have added your recommended changes.

Copy link
Collaborator

@anushbhatia anushbhatia left a comment

Choose a reason for hiding this comment

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

Double-clicking it goes away. So try resolving that cause if the user sets something and clicks it again it shouldn't go away.

import { Tooltip, Avatar } from "@material-ui/core";
import { withStyles, makeStyles } from "@material-ui/core";

const CustomTooltip = withStyles({
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should be defined in CSS.

Copy link
Author

@goyal-Dushi goyal-Dushi Jul 6, 2021

Choose a reason for hiding this comment

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

Hey, @anushbhatia, these are all material imports, thus, they are imported within the component jsx. Check below for the usage.
https://material-ui.com/getting-started/usage/

@goyal-Dushi
Copy link
Author

Double-clicking it goes away. So try resolving that cause if the user sets something and clicks it again it shouldn't go away.

Actually, I have made it so that the user could toggle through it. The user is supposed to click the option once in order to get the option to set values. If you want the option to remain after a single click then no problem, I can add so.

@smaranjitghose
Copy link
Owner

This_should

This should not overlap.

When I assigned the issue, I expected a redesign of the complete menubar and not replacement of certain text with material icons

@goyal-Dushi
Copy link
Author

Hey, @smaranjitghose, I see you have closed the pull request, I can still customize the layout if you want and the overlap issue if you are accepting the pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants