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

refactor(v2/ssr-container): rename ContainerElementFrame to ElementFrame #6551

Merged

Conversation

naorpeled
Copy link
Member

@naorpeled naorpeled commented Jun 15, 2024

Overview

What is it?

  • Feature / enhancement
  • Bug
  • Docs / tests / types / typos

Description

During a session with @mhevery and @shairez,
@mhevery said that the Container prefix is redundant and a bit confusing.
Therefore I created this PR to remove it.

Checklist:

  • My code follows the developer guidelines of this project
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • Added new tests to cover the fix / functionality

@naorpeled naorpeled requested review from mhevery, shairez and a team as code owners June 15, 2024 17:46
Copy link

netlify bot commented Jun 15, 2024

👷 Deploy request for qwik-insights pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 2d3530e

@naorpeled naorpeled changed the base branch from main to build/v2 June 15, 2024 17:46
@naorpeled naorpeled marked this pull request as draft June 15, 2024 17:46
@naorpeled naorpeled changed the title refactor(v2/ssr-container): rename ContainerElementFrame to ElementFrame" refactor(v2/ssr-container): rename ContainerElementFrame to ElementFrame Jun 15, 2024
@naorpeled naorpeled marked this pull request as ready for review June 15, 2024 18:04
Copy link
Member

@thejackshelton thejackshelton left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@gioboa
Copy link
Member

gioboa commented Jun 18, 2024

@naorpeled can you resolve the conflicts please?

@naorpeled
Copy link
Member Author

@naorpeled can you resolve the conflicts please?

Sure thing

@PatrickJS
Copy link
Member

should openContainer and closeContainer be changed?

@naorpeled naorpeled closed this Jun 22, 2024
@naorpeled naorpeled force-pushed the refactor/v2/v2-ssr-container/some-renaming branch from a67d371 to 6ea7260 Compare June 22, 2024 00:13
@naorpeled naorpeled reopened this Jun 22, 2024
@naorpeled naorpeled force-pushed the refactor/v2/v2-ssr-container/some-renaming branch from a67d371 to 2d3530e Compare June 22, 2024 00:19
@naorpeled naorpeled force-pushed the refactor/v2/v2-ssr-container/some-renaming branch from 2d3530e to 8be88e2 Compare June 22, 2024 00:21
@naorpeled
Copy link
Member Author

naorpeled commented Jun 22, 2024

@naorpeled can you resolve the conflicts please?

done. sorry for the delay.

@naorpeled
Copy link
Member Author

should openContainer and closeContainer be changed?

Great question,
not sure, wdyt?

@Varixo
Copy link
Member

Varixo commented Jun 23, 2024

should openContainer and closeContainer be changed?

Great question, not sure, wdyt?

the open and closeContainer are not related to the ContainerElementFrame

@shairez
Copy link
Contributor

shairez commented Jun 23, 2024

thank you @naorpeled ! 🙏

@shairez shairez merged commit f4b5b11 into QwikDev:build/v2 Jun 23, 2024
18 checks passed
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.

6 participants