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

compose: Add stream privacy decorations in banner box. #20562

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

srdeotarse
Copy link
Collaborator

This PR is for issue #19878
Fixes part (1/2) -
Add stream privacy decorations in banner box.

Part (2/2) in another PR as suggested by @alya in Zulip chat https://chat.zulip.org/#narrow/stream/9-issues/topic/missing.20hash.20before.20stream.20name/near/1261461 -
Add stream privacy decorations in compose box.

Testing plan:
Lint test.
Node test.

GIFs or screenshots:
image

Changes explained:

Earlier sent message notification - "Sent! Your message is outside current narrow. Narrow to test > blue"

Updated sent message notification - "Sent! Your message is outside current narrow. Narrow to #test > blue"

@alya
Copy link
Contributor

alya commented Dec 14, 2021

Hm, that hash looks overly large to me.

Could you also:

  1. Add screenshots for a private stream and a world public stream?
  2. Make sure your PR passes checks.

Thanks!

@srdeotarse
Copy link
Collaborator Author

Hm, that hash looks overly large to me.

Could you also:

  1. Add screenshots for a private stream and a world public stream?
  2. Make sure your PR passes checks.

Thanks!

Yes, I will adjust the hash size and also add screenshots for mentioned cases.
I will make sure that my PR passes checks as well.

Thanks for reviewing my PR.

@srdeotarse srdeotarse force-pushed the issue-19878-stream-privacy-decorations-banner-box branch from 16d3dc7 to 2b1ed24 Compare December 14, 2021 17:52
@srdeotarse srdeotarse changed the title Added stream privacy decorations in banner box. Add stream privacy decorations in banner box. Dec 14, 2021
@srdeotarse
Copy link
Collaborator Author

Hm, that hash looks overly large to me.

Could you also:

  1. Add screenshots for a private stream and a world public stream?
  2. Make sure your PR passes checks.

Thanks!

Fix vertical alignment and size of stream privacy decoration icons in banner box.

Screenshots-

Public stream -
image

Private stream -
image

Web Public stream-
image

@raghupalash
Copy link
Collaborator

Thanks for working on this @srdeotarse! I have a review regarding the commits.
Your commit messages don't comply with the commit discipline guide.

Also you might want to clean up your commit history, check out - https://zulip.readthedocs.io/en/latest/git/fixing-commits.html.

Goodday! 😸 🐋

@srdeotarse srdeotarse force-pushed the issue-19878-stream-privacy-decorations-banner-box branch from 25ab9a7 to 692b6b0 Compare December 29, 2021 18:49
@srdeotarse
Copy link
Collaborator Author

@raghupalash I have cleaned my commit history and changed commit messages according to commit guidelines.

@srdeotarse srdeotarse changed the title Add stream privacy decorations in banner box. compose: Add stream privacy decorations in banner box. Jan 6, 2022
@alya
Copy link
Contributor

alya commented Jan 6, 2022

Thanks! It looks to me like there's not enough spacing to the left of the lock and globe icons -- there should be as much space as you normally have between words.

For the hash, can we try having less space between the # and the name of the stream?

@srdeotarse srdeotarse force-pushed the issue-19878-stream-privacy-decorations-banner-box branch from 49d98ff to a7add53 Compare January 7, 2022 12:49
@srdeotarse
Copy link
Collaborator Author

Thanks! It looks to me like there's not enough spacing to the left of the lock and globe icons -- there should be as much space as you normally have between words.

For the hash, can we try having less space between the # and the name of the stream?

Adjusted spacing to the left of globe and lock icons in banner box. Also, reduced spacing between # and name of stream.

@alya
Copy link
Contributor

alya commented Jan 12, 2022

@srdeotarse Could you add updated screenshots here? Thanks!

@srdeotarse
Copy link
Collaborator Author

@alya Adding the updated screenshots -

Added spacing to the left of lock icon -
image

Added spacing to the left of globe icon -
image

Reduced spacing between hash and stream name -
image

@raghupalash
Copy link
Collaborator

@raghupalash I have cleaned my commit history and changed commit messages according to commit guidelines.

I think you'd want to squash all the commits in one single commit (as they all belong to a single change). This might help https://zulip.readthedocs.io/en/latest/git/fixing-commits.html#fixing-the-last-commit. Feel free to bump a thread on the development-help stream if you can't figure it out.

@@ -571,6 +571,14 @@ export function get_stream_privacy_policy(stream_id) {
return stream_privacy_policy_values.private_with_public_history.code;
}

export function get_invite_only(stream_name) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

is_invite_only_by_stream_name function on line 582 does the exact same thing as this, infact is_invite_only_by_stream_name was formerly named as get_invite_only and renamed by Tim on commit 3f4d66109be0265a45713419bbee14394c98414f. So I think we should use the already available helper method instead of making a new one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have removed get_invite_only function and used is_invite_only_by_stream_name.

{defaultMessage: "Narrow to {message_recipient}"},
{message_recipient: get_message_header(message)},
);
const default_message = "Narrow to";
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think $t was used here for translation purposes, so I think we should still stick with using it, plus I don't think your implementation requires you to remove it anyways, you can just let that line be and use it in the compose_notification template as it was used before with your additional changes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Now I have used $t along with my changes.

@srdeotarse srdeotarse force-pushed the issue-19878-stream-privacy-decorations-banner-box branch 4 times, most recently from 5aa4d3a to feff36f Compare January 24, 2022 05:26
@alya
Copy link
Contributor

alya commented Jun 29, 2022

@amanagr perhaps you could review and/or finish up this PR as well?

static/styles/compose.css Outdated Show resolved Hide resolved
@amanagr amanagr force-pushed the issue-19878-stream-privacy-decorations-banner-box branch from feff36f to 9c4a45a Compare June 29, 2022 10:01
@zulipbot zulipbot added size: XL and removed size: L labels Jun 29, 2022
@amanagr
Copy link
Member

amanagr commented Jun 29, 2022

I pushed the commits from #20588 here as well so that they are easier to merge and review.

@amanagr
Copy link
Member

amanagr commented Jun 29, 2022

I removed the underline on link hover since it was not playing nicely with the icons. It just has an increased opacity effect on hover now.
Screenshot 2022-06-29 at 3 16 52 PM

Compose decorations and banner decorations.
Screenshot 2022-06-29 at 3 16 22 PM
Screenshot 2022-06-29 at 3 16 37 PM
Screenshot 2022-06-29 at 3 16 29 PM

@amanagr
Copy link
Member

amanagr commented Jun 29, 2022

@srdeotarse please fetch this PR locally and change author of the last 2 commits to yourself.

@amanagr amanagr force-pushed the issue-19878-stream-privacy-decorations-banner-box branch from 9c4a45a to 2138e27 Compare June 29, 2022 10:19
We will be using them in multiple places inside compose so it
makes sense to have them as a class instead of an id.
@amanagr amanagr force-pushed the issue-19878-stream-privacy-decorations-banner-box branch 2 times, most recently from a230b3a to 3b1a5d8 Compare June 29, 2022 10:25
@srdeotarse srdeotarse force-pushed the issue-19878-stream-privacy-decorations-banner-box branch from 3b1a5d8 to f3fd8ac Compare June 29, 2022 14:01
@srdeotarse
Copy link
Collaborator Author

@amanagr I have changed the author of the last two commits to me.

@amanagr
Copy link
Member

amanagr commented Jun 30, 2022

@srdeotarse did you review the changes I made? Do they look good to you?

@srdeotarse
Copy link
Collaborator Author

@amanagr Yes, separate classes for lock_icon and globe_icon will be helpful for future use as well. Also, my commit from other closed PR is quite similar to this PR, so it makes sense to use them in this PR.

@alya
Copy link
Contributor

alya commented Jul 7, 2022

Thanks for updating this!

I'm not sure about the globe and lock, but the font on # looks odd to me. Could we make it less bold to match the rest of the text?

@alya
Copy link
Contributor

alya commented Jul 7, 2022

Seeing the icons added, it now feels odd not to have the appropriate stream privacy icon in a couple of other places in the compose box area:

  • The "Message..." button
    Screen Shot 2022-07-07 at 10 44 12 AM

  • The "Message..." placeholder text
    Screen Shot 2022-07-07 at 10 44 19 AM

Would it be possible to make these be based on the stream privacy setting as well?

@zulipbot
Copy link
Member

Heads up @srdeotarse, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/main branch and resolve your pull request's merge conflicts accordingly.

@alya
Copy link
Contributor

alya commented Dec 6, 2022

@srdeotarse , will you have time to address the remaining feedback on this PR, or should @amanagr take over from here?

@alya
Copy link
Contributor

alya commented Apr 24, 2023

Let's just complete the banner icon changes in this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
completion candidate PRs with reviews that may unblock merging has conflicts size: XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants