feat: add tabs #10

Majortheus · 2023-02-17T19:17:15Z

Based on this video where @diego3g suggests to make this site close to a real visual code, and one of the suggestions is to have tabs, so tried to have a look on what i could come up with, and this is the result

Preview:

vercel · 2023-02-17T19:17:20Z

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

Name	Status	Preview	Comments	Updated
fala-dev	✅ Ready (Inspect)	Visit Preview	💬 Add your feedback	Feb 22, 2023 at 3:43PM (UTC)

diego3g · 2023-02-17T23:40:07Z

First of all, WTF, this is awesome. I'll take a look at it in a couple of days and add my comments here. Thanks!

diego3g

Visually, this PR is awesome, I don't have any points on how this could be better designed, great job!

About the implementation, I'll leave some comments on the code so we can make it even better!

Also, can you please remove yarn.lock and install the extensions using pnpm install as we are not using Yarn on this application?

diego3g · 2023-02-17T23:44:37Z

src/app/layout.tsx

@@ -1,13 +1,15 @@
-import '../styles/global.css'
+import '../styles/global.css';


Can you take a look at why your editor added this semicolons at the end of these lines?

diego3g · 2023-02-19T12:53:54Z

src/app/layout.tsx


 import { Menu } from '@/components/Menu'
 import { Header } from '@/components/Header'
 import { Explorer } from '@/components/Explorer'
 import { Footer } from '@/components/Footer'
+import { Tabs } from '@/components/Tabs'
+import { TabsProvider } from '@/hooks/useTabs'


Instead of useTabs, maybe we could call this useOpenFiles on the hook as we are storing what files are open.

I always try to look to the future and tabs is a generic name that we can use in other places of the application.

diego3g · 2023-02-19T12:54:07Z

src/app/layout.tsx

@@ -29,20 +31,22 @@ export default function RootLayout({
  return (
    <html lang="en" className={inter.className}>
      <head />
-      <body className="bg-[#7F7FD5] bg-app">
+      <body className="bg-[#7F7FD5] bg-app vsc-initialized">


What is vsc-initialized?

I have some chrome extension that adds this class to the body, and I was having some hydration erros popup on console, so i added to focus on what was really wrong and forgot to remove it afterwards... 🤦

diego3g · 2023-02-19T12:55:05Z

src/app/layout.tsx

        <div className="z-10 relative h-screen p-20 flex items-center justify-center">
          <div className="bg-[#232135] overflow-hidden border border-[#72707D] w-full max-w-[1480px] aspect-video shadow-md shadow-black/20 rounded-lg grid grid-rows-layout">
-            <Header />
+            <TabsProvider>


This could also be something like OpenFilesProvider

diego3g · 2023-02-19T12:55:20Z

src/app/layout.tsx

-              <div className="h-full relative">
-                {children}
+                <div className="h-full relative flex flex-col">
+                  <Tabs />


Suggested change

<Tabs />

<OpenFilesTabs />

diego3g · 2023-02-19T13:08:41Z

src/components/Header.tsx

 export function Header() {
+  const { getActiveTabName } = useTabs();


Instead of a function, I think this should be a variable like currentOpenFile

diego3g · 2023-02-19T13:09:18Z

src/components/Header.tsx

  return (
    <div className="flex items-center justify-between px-3">
      <div className="flex items-center gap-2">
        <button type="button" className="w-3 h-3 bg-[#ED6A5E] rounded-full" />
        <button type="button" className="w-3 h-3 bg-[#F4BF4F] rounded-full" />
        <button type="button" className="w-3 h-3 bg-[#61C554] rounded-full" />
      </div>
+      <span className="text-[#908caa] text-sm">
+        {getActiveTabName()} — fala-Dev


Can we show nothing at all if no file is open? (instead of showing only - fala-Dev)

diego3g · 2023-02-19T13:09:48Z

src/components/Tabs/OpenEditorsSubMenu.tsx

+import { usePathname } from 'next/navigation';
+import Link from 'next/link';
+
+export function OpenEditorsSubMenu() {


To keep the name convention I'm suggesting, we could rename this to OpenFilesSubMenu

diego3g · 2023-02-19T13:12:43Z

src/components/Tabs/RemoveTab.tsx

+  index: number;
+};
+
+export function RemoveTab({ isActive, index }: RemoveTabProps) {


Suggested change

export function RemoveTab({ isActive, index }: RemoveTabProps) {

export function CloseFileButton({ isActive, index }: RemoveTabProps) {

diego3g · 2023-02-19T13:17:25Z

src/hooks/useTabs.tsx

+import { explorerFiles } from '@/components/Explorer';
+
+type TabsType = {
+  children: React.ReactNode[];


I don't think we should store the data here as ReactNode, we should instead probably only store a reference to the open file, not all of its props.

We can have somewhere a file with the list of all files we have in the explorer (we already have that at components/Explorer/index.tsx), something like:

export const explorerFiles = { '/vscode/general': { icon: <SomeIcon /> title: 'General', // any other prop } }

And inside the context, when we open a file, we should save only the file reference, something like:

const openFiles = ['/vscode/general', '/vscode/settings']

What do you think?

Majortheus · 2023-02-20T17:28:24Z

Did some changes, most to make the code more close to the suggestions proposed, there are probably more stuff to fix, and I'm happy to do it.

Looking forward for the new review, this is my first public PR, and it's been reviewed by the person I think I learned the most about coding and JS, very glad a I'm able to amaze you and learn a lot, specially about Next.js 13. 😄

pedro-santos-sky · 2023-02-21T17:25:17Z

Amazing job, congrats @Majortheus 👏 👏 !!!

diego3g

Much better! Just some small changes to make it even better :)

diego3g · 2023-02-22T13:15:49Z

src/components/OpenFilesTabs/CloseFileButton.tsx

+    <a
+      onClick={(e) => {
+        e.preventDefault();
+        e.stopPropagation();
+        closeFile(index);
+      }}
+      className="w-[20px] h-[20px] flex justify-center items-center rounded hover:bg-[#817c9c26]"
+    >


Can this be a button so we don't need to add preventDefault or stopPropagation?

diego3g · 2023-02-22T13:18:33Z

src/hooks/useOpenFiles.tsx

+export function OpenFilesProvider({ children }: { children: React.ReactNode }) {
+  const pathName = usePathname();
+
+  const [openFiles, setOpenFiles] = useState<string[]>(() => {


Instead of string[] here, we can use something like Array<keyof typeof explorerFiles> so openFiles will always include only existent files. This will also remove the necessity to add this if and this one.

diego3g · 2023-02-22T13:19:53Z

src/hooks/useOpenFiles.tsx

+      return;
+    }
+
+    setOpenFiles([...openFiles, file]);


Suggested change

setOpenFiles([...openFiles, file]);

setOpenFiles(state => [...state, file]);

diego3g · 2023-02-22T13:21:29Z

src/hooks/useOpenFiles.tsx

+  const currentOpenFile = () => {
+    const openFileHref = openFiles.find((openFile) => pathName === openFile);
+    if (openFileHref) {
+      return explorerFiles[openFileHref];
+    }
+    return null;
+  };


This doesn't have to be a function.

const currentOpenFile = useMemo(() => { const openFileHref = openFiles.find((openFile) => pathName === openFile); if (openFileHref) { return explorerFiles[openFileHref]; } return null; }, [openFiles, pathName])

diego3g · 2023-02-22T14:14:56Z

Hello @Majortheus, great job!

I'm having some issues while trying to open more than one file, can you take a look at it?

When I open a file, it closes the others;
The "Open Editors" submenu closes when it navigates;

CleanShot.2023-02-22.at.11.12.35.mp4

Majortheus · 2023-02-22T14:59:44Z

I tested with Vercel preview link and yep that's happening, but I remember testing before doing the last commit, so I tested in my machine and... it's working as intended... how fun aheuaheu, I will take a closer look, if you have any pointers, pls gimme it, I already tried to use pnpm build and pnpm start instead of dev, but it didn't make any changes, I don't have any idea on why that's happening though.

2023-02-22-11-50-00.mp4

… more commits will come...

… for all the deploys

… all changes that broke deployed env

Majortheus · 2023-02-22T15:49:55Z

Hmmm I don't know what to do anymore, I tried to undo all the changes before this bug, if you look the deploy before the changes I did today, it was working normal, but the first deploy after it started happening, so I undo all changes and it's happening still, I have no clue...

This deploy is working https://fala-5b58z0knk-rocketseat.vercel.app/

Any deploy afterwards it's not, I will take a break, to reset, I will try again later.

RaczoOBY · 2023-02-22T21:35:34Z

I've been looking at some other PRs, and every single new PR has a curious behavior atfer 21/02.
Even if we check out the commit "fix names and auto formatting" and redeploy it, the from next/link is causing hard reloads.

diego3g · 2023-02-23T13:23:46Z

@Majortheus @RaczoOBY probably some problem with Vercel? 🤔

diego3g · 2023-02-23T13:26:32Z

I tested it on my local environment and it works properly. I think there ir something wrong with preview deployments on Vercel so I will merge this.

Majortheus added 4 commits February 17, 2023 14:58

feat: added tabs

15b1985

fix: formatting

2f52350

fix: more formatting

afbedf5

fix: weird bug when closing active tab

1fa491e

vercel bot deployed to Preview February 17, 2023 19:18 View deployment

diego3g requested changes Feb 19, 2023

View reviewed changes

fix: naming and types

4ce8476

vercel bot deployed to Preview February 20, 2023 16:58 View deployment

chore: naming

850902d

vercel bot deployed to Preview February 20, 2023 17:18 View deployment

lint: fix names and auto formatting

8316587

Majortheus requested a review from diego3g February 20, 2023 17:20

vercel bot deployed to Preview February 20, 2023 17:20 View deployment

diego3g requested changes Feb 22, 2023

View reviewed changes

chore: small changes to improve code quality and readability

cdf2d2a

vercel bot deployed to Preview February 22, 2023 14:06 View deployment

Majortheus requested a review from diego3g February 22, 2023 14:06

diego3g approved these changes Feb 22, 2023

View reviewed changes

fix: tring to fix wierd bug thats happening only in production, maybe…

e8a2fa0

… more commits will come...

vercel bot deployed to Preview February 22, 2023 15:23 View deployment

fix: tring to fix wierd bug thats happening only in production, maybe…

271e39f

… more commits will come...

vercel bot deployed to Preview February 22, 2023 15:28 View deployment

fix: tring to fix wierd bug thats happening only in production, maybe…

bdb0e18

… more commits will come...

vercel bot deployed to Preview February 22, 2023 15:34 View deployment

fix: tring to fix wierd bug thats happening only in production, sorry…

f027687

… for all the deploys

vercel bot deployed to Preview February 22, 2023 15:38 View deployment

fix: tring to fix wierd bug thats happening only in production, undid…

ad6a241

… all changes that broke deployed env

vercel bot deployed to Preview February 22, 2023 15:43 View deployment

diego3g merged commit b5d61b4 into diego3g:main Feb 23, 2023

diego3g added the enhancement New feature or request label Feb 23, 2023

diego3g mentioned this pull request Feb 24, 2023

After selecting an file under folder other than the first one, the folder keeps closing #22

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add tabs #10

feat: add tabs #10

Majortheus commented Feb 17, 2023

vercel bot commented Feb 17, 2023 •

edited

Loading

diego3g commented Feb 17, 2023

diego3g left a comment •

edited

Loading

diego3g Feb 17, 2023

diego3g Feb 19, 2023

diego3g Feb 19, 2023

Majortheus Feb 20, 2023

diego3g Feb 19, 2023

diego3g Feb 19, 2023

diego3g Feb 19, 2023

diego3g Feb 19, 2023

diego3g Feb 19, 2023

diego3g Feb 19, 2023

diego3g Feb 19, 2023

Majortheus commented Feb 20, 2023

pedro-santos-sky commented Feb 21, 2023

diego3g left a comment

diego3g Feb 22, 2023

diego3g Feb 22, 2023

diego3g Feb 22, 2023

diego3g Feb 22, 2023

diego3g commented Feb 22, 2023

Majortheus commented Feb 22, 2023

Majortheus commented Feb 22, 2023

RaczoOBY commented Feb 22, 2023

diego3g commented Feb 23, 2023

diego3g commented Feb 23, 2023

		@@ -1,13 +1,15 @@
		import '../styles/global.css'
		import '../styles/global.css';

		export function Header() {
		const { getActiveTabName } = useTabs();

	export function RemoveTab({ isActive, index }: RemoveTabProps) {
	export function CloseFileButton({ isActive, index }: RemoveTabProps) {

	setOpenFiles([...openFiles, file]);
	setOpenFiles(state => [...state, file]);

feat: add tabs #10

feat: add tabs #10

Conversation

Majortheus commented Feb 17, 2023

vercel bot commented Feb 17, 2023 • edited Loading

diego3g commented Feb 17, 2023

diego3g left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Majortheus commented Feb 20, 2023

pedro-santos-sky commented Feb 21, 2023

diego3g left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diego3g commented Feb 22, 2023

Majortheus commented Feb 22, 2023

Majortheus commented Feb 22, 2023

RaczoOBY commented Feb 22, 2023

diego3g commented Feb 23, 2023

diego3g commented Feb 23, 2023

vercel bot commented Feb 17, 2023 •

edited

Loading

diego3g left a comment •

edited

Loading