-
Notifications
You must be signed in to change notification settings - Fork 22
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
add latency to terminal window #463
Conversation
WalkthroughThe recent updates focus on refining the user interface and interactivity within a web application. Key adjustments include the repositioning and restyling of elements for better visual alignment and responsiveness, particularly in smaller screens. Additionally, there's a simplification in the display of statistics, emphasizing essential data. These changes aim to enhance user experience by improving layout fluidity and data presentation. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
✅ Deploy Preview for terminal7 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for dazzling-kringle-090742 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (3)
- css/terminal7.css (5 hunks)
- src/gate.ts (1 hunks)
- src/map.ts (1 hunks)
Additional comments not posted (5)
src/map.ts (1)
222-222
: Ensure thatstats.roundTripTime
is properly sanitized before being inserted into the DOM to prevent XSS attacks.css/terminal7.css (3)
306-306
: The change to.windows-container
positioning aligns well with the PR's objectives of enhancing the terminal window's layout and appearance.
534-534
: The addition of.search-container
with specific styling for margin and width supports the PR's goal of improving UI responsiveness and appearance.
897-917
: Adjustments to media queries for screens under 600px width enhance the application's responsiveness and usability on mobile devices.src/gate.ts (1)
448-453
: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
Verify the impact of removing
container.style.top = "22px"
on the layout and positioning of elements within theGate
class to ensure it aligns with the intended objectives.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
…into latency-branch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (3)
- css/terminal7.css (5 hunks)
- src/gate.ts (1 hunks)
- src/map.ts (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- css/terminal7.css
- src/gate.ts
- src/map.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (2)
- css/terminal7.css (9 hunks)
- src/gate.ts (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- css/terminal7.css
- src/gate.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! It's almost there...
@@ -897,4 +888,17 @@ video { | |||
opacity: 0.8; | |||
z-index: 3; | |||
} | |||
|
|||
@media (max-width: 600px) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you move this section here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is customary for CSS files to be ordered by media query sizes. So when I started studying the app, I put it at the bottom as a best practice habit, it serves no actual purpose.
src/gate.ts
Outdated
@@ -448,7 +448,7 @@ export class Gate { | |||
container.style.removeProperty("height") | |||
scale = 1 | |||
container.style.left = "0" | |||
container.style.top = "22px" | |||
container.style.top = "0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this line necessary? We already have top: 0 in the css
css/terminal7.css
Outdated
@@ -767,11 +749,20 @@ audio { display: none; } | |||
text-align: left; | |||
padding-left: 1.5em; | |||
text-shadow: none; | |||
z-index: 20; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
css/terminal7.css
Outdated
@@ -767,11 +749,20 @@ audio { display: none; } | |||
text-align: left; | |||
padding-left: 1.5em; | |||
text-shadow: none; | |||
z-index: 20; | |||
border-bottom: 1px rgba(255, 255, 255, 0.7) solid; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use the current border color of we use the separate buttons on the bnav bar: rgba(55, 55, 2, 1)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will change it
the root color is --dark-border
css/terminal7.css
Outdated
border-bottom: 1px rgba(255, 255, 255, 0.7) solid; | ||
border-right: 0; | ||
border-top: 0; | ||
-webkit-border-top-right-radius: 6px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This and the next line are not needed anymore. It's an ancient hack and all modern browsers have support for border-top-right-radius
. In the future, use https://caniuse.com to check if you need any browser specific rules
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear, what lines we're talking about
- 4th line will be changed to border-top-right-radius
- lines 2-3 can be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (2)
- css/terminal7.css (9 hunks)
- src/gate.ts (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- css/terminal7.css
- src/gate.ts
Thanks! I merged manually |
Summary by CodeRabbit
.windows-container
,.search-container
, and.gate
in the CSS file.Gate
class for improved alignment.