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

feat: Rename new default theme #771

Merged
merged 9 commits into from
Oct 2, 2023

Conversation

AdamJaggard
Copy link
Contributor

@AdamJaggard AdamJaggard commented Sep 15, 2023

classic is now daniel, 2023 is now gerwig

This is technically a breaking change but because no one is likely aware that these recent exports exist we can decide how we want to handle merging. Seeing as switching to the new default theme is going to be a major version bump anyway we may want to hold off merging until we are preparing for that release?

Old theme is remaining as classic, new theme is now gerwig.

@AdamJaggard AdamJaggard requested a review from a team as a code owner September 15, 2023 10:27
@vercel
Copy link

vercel bot commented Sep 15, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
elements-demo-create-react-app ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 25, 2023 2:26pm
elements-demo-nextjs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 25, 2023 2:26pm
elements-demo-svelte-kit ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 25, 2023 2:26pm
elements-demo-vanilla ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 25, 2023 2:26pm
elements-demo-vue ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 25, 2023 2:26pm

@codecov
Copy link

codecov bot commented Sep 15, 2023

Codecov Report

Merging #771 (d830b19) into main (4f1b684) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #771   +/-   ##
=======================================
  Coverage   82.16%   82.16%           
=======================================
  Files          40       40           
  Lines        7868     7868           
  Branches      457      457           
=======================================
  Hits         6465     6465           
  Misses       1397     1397           
  Partials        6        6           

luwes
luwes previously requested changes Sep 19, 2023
Copy link
Contributor

@luwes luwes left a comment

Choose a reason for hiding this comment

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

could we have a few more people review this? @heff @mmcc

it's in fun spirit which I like but it feels disconnected to me.
to me it didn't ring any bells, daniel or gerwig.

also how do these fit with the other themes minimal and microvideo?
in the future in player.style?

I'm leaning more to keep classic and just a new catchy name for the new theme.

it's pink and bubbly. gerwig doesn't feel very catchy to me

@cjpillsbury
Copy link
Contributor

could we have a few more people review this? @heff @mmcc

Happy to get input from 👆, but since this is naming the official elements themes, I believe Dylan would be a a more appropriate reviewer/decision maker to loop in.

it's in fun spirit which I like but it feels disconnected to me. to me it didn't ring any bells, daniel or gerwig.

This was based on a conversation with @dylanjha and I had that some other folks chimed in on out of band. The idea was taking our own spin on codenames like e.g. Android's, where the name is honoring a writer/director of a movie from the year that the theme was created (to that end, I'm not sure if we settled on "daniel" vs. "kwan" out of band). Like the Android or other naming conventions, they typically aren't going to have hard associations outside of the place that's establishing them (what does "eclair" or "froyo" mean outside of the fact that folks expect Android OS codenames to be desert-y? Will it tell you which version something is, or when it was made, etc.?).

also how do these fit with the other themes minimal and microvideo?
in the future in player.style?

We've previously decided that we were going to treat the "official" mux player themes differently from "generic media chrome themes," not just in where the code for the themes live (part of elements/mux player package as opposed to e.g. media chrome), but in how we will/won't treat them as "readily usable for things other than mux player". Even though we designed things like the minimal and microvideo themes, these were created as generic media chrome themes, so I wouldn't say we need to worry about naming consistency there.

@cjpillsbury
Copy link
Contributor

It's also worth noting that we could also treat these as "aliases" and simply keep the year or some other versioning in place so folks could use either/both to reference their theme, whatever "fun codename" we decide to assign.

@dylanjha
Copy link
Contributor

dylanjha commented Sep 20, 2023

I'm leaning more to keep classic and just a new catchy name for the new theme.

@luwes -- can you make a suggestion for a new name?

it's pink and bubbly. gerwig doesn't feel very catchy to me

Gerwig is a reference to Greta Gerwig, which is the director of Barbie (pink and bubbly), seems okay to me?

I'm open to other suggestions. Can also go with classic and gerwig because the other one daniels is a bit of a stretch

@AdamJaggard
Copy link
Contributor Author

I think keeping classic and naming the new one gerwig is a good middle ground. Adding aliases feels a bit messy.

@luwes
Copy link
Contributor

luwes commented Sep 20, 2023

@cjpillsbury yep, I saw the Slack convo's. that's why I didn't cc Dylan because he's in the loop.

pink, bubbly -> barbie -> gerwig is a step too far IMO.
I don't think 50% gets the reference. at least with eclair, froyo something could be visualized

why not call it barbie then? easier to remember

other suggestions: bubblegum, rosa (rose), pinkpop, pinkfizz, jellyfish,
https://www.google.com/search?q=what%27s+pink+in+nature

@luwes luwes dismissed their stale review September 21, 2023 14:46

gerwig for the new theme I can adapt too

@cjpillsbury
Copy link
Contributor

Cool. No notes on my side.

@AdamJaggard
Copy link
Contributor Author

@cjpillsbury, @dylanjha, @luwes, I've reverted old theme back to classic, new theme is still gerwig.

As this now only affects the new theme, it's a safer merge.

Copy link
Contributor

@luwes luwes left a comment

Choose a reason for hiding this comment

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

LGTM 👍

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

Successfully merging this pull request may close these issues.

4 participants