Skip to content
This repository was archived by the owner on Jan 22, 2026. It is now read-only.

[ch18573] Add new icons for project privacy.#122

Merged
Osmose merged 1 commit intomasterfrom
privacy-icons
Sep 17, 2020
Merged

[ch18573] Add new icons for project privacy.#122
Osmose merged 1 commit intomasterfrom
privacy-icons

Conversation

@Osmose
Copy link
Copy Markdown
Contributor

@Osmose Osmose commented Sep 16, 2020

This adds the 3 new icons for project privacy to the shared icon list for use in the Editor and Community sites. I added them as new icon names without overwriting the existing private and public icons because:

  1. I think our icons should describe what they depict instead of what we use the symbol to mean.
  2. It will let us use the new icons without modifying the old ones.

I took the SVGs from https://github.com/glitchdotcom/Glitch-Editor/pull/898/, but had to modify the codeLock icon a bit to use clipping instead of a background color to make the code symbol work.

@Osmose Osmose requested a review from sheridanvk September 16, 2020 06:48
Copy link
Copy Markdown
Contributor

@sheridanvk sheridanvk left a comment

Choose a reason for hiding this comment

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

looks great, I can modify my PR to use these once you've shipped the new library version. I can also put the component in the shared library so you can just use that, what do you think?

@Osmose
Copy link
Copy Markdown
Contributor Author

Osmose commented Sep 16, 2020

looks great, I can modify my PR to use these once you've shipped the new library version. I can also put the component in the shared library so you can just use that, what do you think?

I think just the icons is just fine, keeping the project-object-specific logic outside shared-components seems cleaner to me.

@Osmose Osmose merged commit 157c888 into master Sep 17, 2020
@Osmose Osmose deleted the privacy-icons branch September 17, 2020 06:41
@sheridanvk
Copy link
Copy Markdown
Contributor

hmm but you'll need to use the same logic in the Community site, so that seems repetitive. I was thinking the shared-components library is useful for that kind of thing too; do you disagree? It's a pretty clean API; just takes in a privacy string and returns the appropriate SVG (we can add in proptypes for more specificity on what you can pass in too)

@Osmose
Copy link
Copy Markdown
Contributor Author

Osmose commented Sep 17, 2020

I think logic that relies on our data model (i.e. data returned from the API) doesn't really fit in shared-components; most everything else in here currently is more generic than that. Making shared components start to rely on the shape of data returned from the API limits our options for state management in the community and editor sites (e.g. we wouldn't be able to add client-side model wrappers around API responses if we wanted to in the future).

Returning the right SVG based on a string doesn't really violate that, but at the same time it's also a small enough helper to write that there's not really much benefit in sharing the implementation between both sites to offset the cost of having another codebase to consider when figuring out what the components do.

@keithk
Copy link
Copy Markdown
Contributor

keithk commented Nov 10, 2020

🚀 PR was released in v0.19.0 🚀

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants