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

Refactoring subscription modal checkmarks to icon format #11328

Closed
wants to merge 3 commits into from

Conversation

synicalsyntax
Copy link
Member

Turns subscription modal checkmarks into custom icons to fix #10918. Involves a significant amount of CSS refactoring.

Before:
screenshot at jan 21 16-25-37

After:
screenshot at jan 21 16-25-20

Also includes miscellaneous documentation changes, including custom icon webfont documentation in frontend build processes article and fixing discrepancies in the SVG test tool description.

@zulipbot
Copy link
Member

Hello @zulip/server-streams members, this pull request was labeled with the "area: stream settings" label, so you may want to check it out!

@timabbott
Copy link
Sponsor Member

Does this also change the ones in e.g. the "stream description" field?

@synicalsyntax
Copy link
Member Author

@timabbott No, the styling of that checkmark is different and isn't rendered as an SVG (it's text: ).

}

#subscription_overlay .stream-description .stream-description-editable:empty::after,
Copy link
Sponsor Member

@timabbott timabbott Feb 5, 2019

Choose a reason for hiding this comment

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

This line seems to have been split with a comma? How did you generate this commit ? I would have imagined one would have done it with just copy-pasting rearranging things to avoid risk of introducing unexpected changes like this.

#subscription_overlay .stream-description,
.stream-description-editable:empty::after {

If you're doing more complex changes to some of the blocks to re-split them or something, please do that in a separate commit from simple reordering -- it's really unpleasant to verify a reordering commit squashed with other changes.

@synicalsyntax
Copy link
Member Author

@timabbott Added a separate commit for splitting out the selectors like you asked, please review!

@timabbott
Copy link
Sponsor Member

I merged the first commit. For subs: Split stream row selectors for SASS nested selector feature., it seems like you lost this block:

-.stream-row::-moz-selection,
-.stream-row .icon .hashtag::-moz-selection {
+.stream-row::-moz-selection {
     background-color: transparent;
 }
 
-.stream-row::selection,
-.stream-row .icon .hashtag::selection {
+.stream-row::selection {
     background-color: transparent;
 }

(At least, that change isn't a no-op)

margin: 0% 15%;
}

&.checked {
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

I think at least this use of & is buggy, see https://css-tricks.com/the-sass-ampersand/. The original rule here was
.stream-row .checked svg, not .stream-row.checked svg which is what you have here...

Copy link
Member Author

Choose a reason for hiding this comment

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

No, I believe I didn't make a mistake here. The original CSS was:

.stream-row .check svg {
    fill: transparent;
    width: 70%;
    margin: 0% 15%;
}

.stream-row .checked svg {
    fill: hsl(170, 48%, 54%);
}

.stream-row .check.checked:hover svg {
    opacity: 0.5;
}

The DOM looks like:

<div class="stream-row" data-stream-id="1" data-stream-name="announce">
    <div class="check checked sub_unsub_button">

So, really, .stream-row .checked svg should be .stream-row .check.checked svg. Also, & refers to the parent selector of the current element, which is .check and not .stream-row since it's nested under the .check selector which is nested under .stream-row.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Yes, but if I'm not mistaken, &.checked is how you generate .stream-row.checked, not .stream-row .checked - i.e. expecting the checked to be on the same element as the stream-row.

@synicalsyntax
Copy link
Member Author

@timabbott Actually, I split that block out and moved it up:
screenshot at feb 18 16-57-08

@zulipbot
Copy link
Member

zulipbot commented May 8, 2019

Heads up @synicalsyntax, 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/master branch and resolve your pull request's merge conflicts accordingly.

@timabbott
Copy link
Sponsor Member

@synicalsyntax did you mean to close this? If you don't have time for it, maybe @YJDave can pick it up.

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.

Create an icon font for Manage Stream checkmarks.
3 participants