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

[GH-535] Add non-breaking space before 'show description button' #604

Closed
wants to merge 8 commits into from

Conversation

lunaticmonk
Copy link

@lunaticmonk lunaticmonk commented Jun 19, 2021

Summary

Adds a space between the "show description" description and the icon on a board.

image

Ticket Link

#535

@lunaticmonk lunaticmonk changed the title Add non-breaking space before 'show description button' [GH-535] Add non-breaking space before 'show description button' Jun 19, 2021
@lunaticmonk
Copy link
Author

@chenilim I have signed the CLA but I don't know how to re-trigger the workflows to let the cla/mattermost workflow pass. Can you please help me out?

@chenilim
Copy link
Contributor

/check-cla

@lunaticmonk
Copy link
Author

@chenilim Can you please review?

image

@@ -64,6 +64,7 @@ const ViewTitle = React.memo((props: Props) => {
}}
icon={<ShowIcon/>}
>
&nbsp;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't love the use of &nbsp; here, it remove certain control of the styling, do you mind to add a .ViewTitle .add-buttons .Icon css that add a margin-right, that way we can make more fine grain styling of this.

Copy link
Contributor

Choose a reason for hiding this comment

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

by the way, the right place would be the file webapp/src/components/viewTitle.scss

Copy link
Contributor

@jespino jespino left a comment

Choose a reason for hiding this comment

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

I prefer to go with css intead of &nbsp for this visual bug.

@mattermod
Copy link
Contributor

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

/cc @jasonblais @jfrerich @emilyacook

@harshilsharma63
Copy link
Member

@lunaticmonk just checking in if you have any update on this?

@lunaticmonk lunaticmonk requested a review from a team as a code owner July 11, 2021 10:33
@lunaticmonk lunaticmonk requested review from sbishel and ogi-m July 11, 2021 10:33
@lunaticmonk
Copy link
Author

@harshilsharma63 @jespino Pushed the css changes. Please review once.

@@ -38,4 +38,8 @@
> .description > * {
flex-grow: 1;
}

.non-breaking-space {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not what I was expecting, I was thinking more in something like changing the already existing .add-buttons .Icon class and add there the margin-right: 0.5em, that way you don't need the span element at all, and you also fix the same problem with the Hide and Emoji icons.

@sbishel
Copy link
Collaborator

sbishel commented Jul 29, 2021

@lunaticmonk Just checking in if you are still working on this.

@lunaticmonk
Copy link
Author

@sbishel Yes, will update this PR soon, was a bit busy from few days at work!

@lunaticmonk
Copy link
Author

@sbishel I accidently logged out of my account on my local setup and can't remember the password. Is there a way to reset the password via some hack? I need to test these changes once. I also tried signing up again but getting:

image

Can you help me in this?

@sbishel
Copy link
Collaborator

sbishel commented Aug 19, 2021

@lunaticmonk Sorry, I missed this message. No, we don't have a hack for that. You could hack the change password code (ie, don't require old password) and run via ChangePassword API.

I'm not sure why you can't register a new user are you seeing any other errors?

If you want to push it up this PR, I'm happy to test.

@lunaticmonk
Copy link
Author

lunaticmonk commented Sep 1, 2021

@sbishel I tried doing few hacks but the page gets redirected to login and I cannot reset password. Looks like I will need your help in testing this and merging it. Meanwhile, I will set up the repo again from scratch to solve the auth issue.

@sbishel
Copy link
Collaborator

sbishel commented Sep 1, 2021

@lunaticmonk Sure, merge your changes to this PR and I'll test. The only hack would be if your running source code to put a break point in where login fails and go around it.

@lunaticmonk
Copy link
Author

@sbishel Didn't get you? Which PR should I merge my changes to? Is there any other branch you have where I should merge these?

@sbishel
Copy link
Collaborator

sbishel commented Sep 8, 2021

@lunaticmonk This one. I thought you had additional updates to this PR based on Jesus feedback.

@lunaticmonk
Copy link
Author

@sbishel These are the only changes I believe. You can test this branch.

@sbishel
Copy link
Collaborator

sbishel commented Sep 8, 2021

Fixed by #765

@sbishel sbishel closed this Sep 8, 2021
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.

6 participants