1.Added a regex filter for the AIX OS. 2. Added constants for the STORYBOOK_MODE 3.Removed unecessary closing of with or '||' operator #6635

mortale2004 · 2024-04-09T21:18:53Z

Added a regex filter for the AIX OS.

This os was missing in the given regex for finding the current os of the user.
This regex will help to find out the current os by using switch case .

Added constants for the STORYBOOK_MODE

The constants will prevent from typos and we can change the value of it from single place.

Removed unecessary closing of
with or '||' operator.

This can give error in the css styling and unecessary use of or '||' operator.

…OS in sswitch case statement.

vercel · 2024-04-09T21:18:56Z

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

Name	Status	Preview	Updated (UTC)
nodejs-org	✅ Ready (Inspect)	Visit Preview	Apr 10, 2024 2:06pm

ovflowd · 2024-04-09T21:23:50Z

I don't think people download Node for AIX within AIX. It is the sort of enterprise OS that system administrators would connect remotely no?

cc @mhdawson

mortale2004 · 2024-04-09T21:30:06Z

I don't think people download Node for AIX within AIX. It is the sort of enterprise OS that system administrators would connect remotely no?

cc @mhdawson

But there can be possibility of downloading node js on the AIX OS. If this case is not going to occur then we should remove the AIX os from the switch case also isn't it?

ovflowd · 2024-04-09T21:31:24Z

I don't think people download Node for AIX within AIX. It is the sort of enterprise OS that system administrators would connect remotely no?

cc @mhdawson

But there can be possibility of downloading node js on the AIX OS. If this case is not going to occur then we should remove the AIX os from the switch case also isn't it?

Are you sure AIX is part of the user agent for browsers running on AIX? Is there even any modern browser running on AIX?

Do you have example user agents?

ovflowd · 2024-04-09T21:32:17Z

I don't think people download Node for AIX within AIX. It is the sort of enterprise OS that system administrators would connect remotely no?

cc @mhdawson

But there can be possibility of downloading node js on the AIX OS. If this case is not going to occur then we should remove the AIX os from the switch case also isn't it?

The reason why AIX is on the switch is so people can download Node for AIX. Auto detection is just a sugar on the top, that we do for OSs that we know people open on the browser to download Node.

mortale2004 · 2024-04-09T21:38:06Z

I don't think people download Node for AIX within AIX. It is the sort of enterprise OS that system administrators would connect remotely no?

cc @mhdawson

But there can be possibility of downloading node js on the AIX OS. If this case is not going to occur then we should remove the AIX os from the switch case also isn't it?

The reason why AIX is on the switch is so people can download Node for AIX. Auto detection is just a sugar on the top, that we do for OSs that we know people open on the browser to download Node.

In the code that i have made changes. We are just getting the constant name for the Operating Systems from the user agent of the browser and this can be used to give the Node js according to the OS of its system. This is just for the easy accessibility to download Node js.

ovflowd · 2024-04-09T21:39:48Z

I don't think people download Node for AIX within AIX. It is the sort of enterprise OS that system administrators would connect remotely no?

cc @mhdawson

But there can be possibility of downloading node js on the AIX OS. If this case is not going to occur then we should remove the AIX os from the switch case also isn't it?

The reason why AIX is on the switch is so people can download Node for AIX. Auto detection is just a sugar on the top, that we do for OSs that we know people open on the browser to download Node.

In the code that i have made changes. We are just getting the constant name for the Operating Systems from the user agent of the browser and this can be used to give the Node js according to the OS of its system. This is just for the easy accessibility to download Node js.

You haven't answered my question. Is there any real user agent out there that contains "AIX"?

What is the point of adding AIX to the user agent query if there aren't any browser user agents with AIX within the user agent string?

ovflowd

But anyhow, the changes make sense at least in order to make the switch statement work. Although I dont think we should have AIX there to begin with. (In the OS detection, but I might be wrong tho!)

Thanks for the quick fix, appreciate the time you put into this. Ill check with @mhdawson regarding if AIX os detection is intended or not 🤔

mortale2004 · 2024-04-09T21:44:17Z

I don't think people download Node for AIX within AIX. It is the sort of enterprise OS that system administrators would connect remotely no?

cc @mhdawson

But there can be possibility of downloading node js on the AIX OS. If this case is not going to occur then we should remove the AIX os from the switch case also isn't it?

The reason why AIX is on the switch is so people can download Node for AIX. Auto detection is just a sugar on the top, that we do for OSs that we know people open on the browser to download Node.

In the code that i have made changes. We are just getting the constant name for the Operating Systems from the user agent of the browser and this can be used to give the Node js according to the OS of its system. This is just for the easy accessibility to download Node js.

You haven't answered my question. Is there any real user agent out there that contains "AIX"?

What is the point of adding AIX to the user agent query if there aren't any browser user agents with AIX within the user agent string?

Yes, you are right currently there is no any user agent specifiec to the AIX OS but in future it can come. Because AIX is also a popular OS.

…ng or div and or '||' operator from withSidebarCrossLinks.tsx

mortale2004

Added Filter for AIX OS
Used Constants for theme of STORY_BOOK
Removed Extra closing of

…g constant values for the theme modes.

….org into featureBranch

ovflowd · 2024-04-10T06:50:11Z

I see unrelated changes on this PR from the last time Ive approved the PR 🤔

Would you mind putting those changes on another PR as they are unrelated?

mortale2004 · 2024-04-10T06:52:54Z

I see unrelated changes on this PR from the last time Ive approved the PR 🤔

Would you mind putting those changes on another PR as they are unrelated?

Okay I will do that just tell me when this PR will be merged?

ovflowd · 2024-04-10T06:54:32Z

It is written on the CONTRIBUTING.md file, but in general changes require 48h to get merged, but since the original change is simple I can "fast track" this PR which would allow me to merge it immediately

mortale2004 · 2024-04-10T07:17:22Z

It is written on the CONTRIBUTING.md file, but in general changes require 48h to get merged, but since the original change is simple I can "fast track" this PR which would allow me to merge it immediately

Is there need to create different PR for the other commit becuase I have added that all issues commited in the Description of this PR.

ovflowd · 2024-04-10T08:19:57Z

It is written on the CONTRIBUTING.md file, but in general changes require 48h to get merged, but since the original change is simple I can "fast track" this PR which would allow me to merge it immediately

Is there need to create different PR for the other commit becuase I have added that all issues commited in the Description of this PR.

Unfortunately, yes. I can re review this PR with the new changes but with the new changes this PR is not eligible anymore to be fast tracked and it might take time to be reviewed.

mortale2004 · 2024-04-10T08:59:04Z

Ok no problem you can take your time

…

On Wed, 10 Apr, 2024, 1:50 pm Claudio W, ***@***.***> wrote: It is written on the CONTRIBUTING.md file, but in general changes require 48h to get merged, but since the original change is simple I can "fast track" this PR which would allow me to merge it immediately Is there need to create different PR for the other commit becuase I have added that all issues commited in the Description of this PR. Unfortunately, yes. I can re review this PR with the new changes but with the new changes this PR is not eligible anymore to be fast tracked and it might take time to be reviewed. — Reply to this email directly, view it on GitHub <#6635 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/A3ISMXVILQ52L6VHD6IUGN3Y4TY4JAVCNFSM6AAAAABF7JQSGOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANBWHA3TSMBVGY> . You are receiving this because you authored the thread.Message ID: ***@***.***>

mhdawson · 2024-04-10T20:53:08Z

I read this as indicating its unlikely people will be using a brower on AIX to directly access the site- https://www.ibm.com/support/pages/ibm-aix-firefox-web-browser-faq. Instead most likely they will download from another system and transfer to the AIX server.

I'm ok with the autodetection but agree we are likely ok without it.

mortale2004 · 2024-04-11T04:23:56Z

I read this as indicating its unlikely people will be using a brower on AIX to directly access the site- https://www.ibm.com/support/pages/ibm-aix-firefox-web-browser-faq. Instead most likely they will download from another system and transfer to the AIX server.

I'm ok with the autodetection but agree we are likely ok without it.

If you are Ok without autodetection then there is no need to keep AIX in the switch case for the detection of OS. It will won't work in any case because there is no any browser agent for the AIX OS.

ovflowd · 2024-04-15T08:05:41Z

@mortale2004 @mhdawson how y'all want to proceed?

mhdawson · 2024-04-15T21:14:09Z

I'd just leave out the autodection for AIX.

mortale2004 · 2024-04-15T21:17:23Z

Okay no problem

…

On Tue, 16 Apr, 2024, 2:44 am Michael Dawson, ***@***.***> wrote: I'd just leave out the autodection for AIX. — Reply to this email directly, view it on GitHub <#6635 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/A3ISMXSRF3M72AKN5G42ABTY5Q7LPAVCNFSM6AAAAABF7JQSGOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANJXHAZDANRTGI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

ovflowd · 2024-04-21T08:21:48Z

components/withSidebarCrossLinks.tsx

@@ -25,13 +25,13 @@ const WithSidebarCrossLinks: FC<WithCrossLinksProps> = ({ navKey }) => {

  return (
    <div className="mt-4 grid w-full grid-cols-2 gap-4 xs:grid-cols-1">
-      {(previousCrossLink && (
+      {previousCrossLink && (


Afaik the div element is intentional, otherwise the nextCrossLink goes to the left, instead of staying in the right 😅

ovflowd · 2024-04-21T08:22:28Z

util/detectOS.ts

@@ -1,7 +1,7 @@
 import type { UserOS } from '@/types/userOS';

 export const detectOsInUserAgent = (userAgent: string | undefined): UserOS => {
-  const osMatch = userAgent?.match(/(Win|Mac|Linux)/);
+  const osMatch = userAgent?.match(/(Win|Mac|Linux|AIX)/);


I think as mentioned before, instead of adding AIX to this match, we should remove the "case AIX"

ovflowd · 2024-04-21T08:23:10Z

components/withNavBar.tsx

@@ -4,6 +4,7 @@ import { useLocale } from 'next-intl';
 import { useTheme } from 'next-themes';
 import type { FC } from 'react';

+import { STORYBOOK_MODE_THEME } from '@/.storybook/constants';


As mentioned before, we shouldn't have Sotrybook constants being imported in Application code, you can create a new constant similar to the Storybook one, on an App constants file, like next.constants.mjs

Add Missing OS value in the Regex for handling the current userAgent …

22ec8ce

…OS in sswitch case statement.

mortale2004 requested a review from a team as a code owner April 9, 2024 21:18

vercel bot deployed to Preview April 9, 2024 21:20 View deployment

ovflowd approved these changes Apr 9, 2024

View reviewed changes

Added Constants for the STORYBOOK_MODE_THEME and removed extran closi…

5dfa804

…ng or div and or '||' operator from withSidebarCrossLinks.tsx

vercel bot deployed to Preview April 9, 2024 23:03 View deployment

mortale2004 commented Apr 9, 2024

View reviewed changes

mortale2004 changed the title ~~Add Missing OS value in the Regex for handling it in switch case statement for getting current OS of user.~~ 1.Added a regex filter for the AIX OS. 2. Added constants for the STORYBOOK_MODE 3.Removed unecessary closing of with or '||' operator. Apr 9, 2024

netizenlive10 added 2 commits April 10, 2024 06:21

Added STORYBOOK_MODE_THEME in the constants of the storybook for usin…

9e24d8a

…g constant values for the theme modes.

Merge branch 'featureBranch' of https://github.com/mortale2004/nodejs…

e23e272

….org into featureBranch

vercel bot deployed to Preview April 10, 2024 06:12 View deployment

fix: Removed extra closing of div

5bbae24

mortale2004 requested a review from ovflowd April 10, 2024 13:10

vercel bot deployed to Preview April 10, 2024 14:06 View deployment

ovflowd reviewed Apr 21, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

1.Added a regex filter for the AIX OS. 2. Added constants for the STORYBOOK_MODE 3.Removed unecessary closing of with or '||' operator #6635

1.Added a regex filter for the AIX OS. 2. Added constants for the STORYBOOK_MODE 3.Removed unecessary closing of with or '||' operator #6635

mortale2004 commented Apr 9, 2024 •

edited

vercel bot commented Apr 9, 2024 •

edited

ovflowd commented Apr 9, 2024

mortale2004 commented Apr 9, 2024

ovflowd commented Apr 9, 2024

ovflowd commented Apr 9, 2024

mortale2004 commented Apr 9, 2024

ovflowd commented Apr 9, 2024

ovflowd left a comment •

edited

mortale2004 commented Apr 9, 2024

mortale2004 left a comment •

edited

ovflowd commented Apr 10, 2024

mortale2004 commented Apr 10, 2024

ovflowd commented Apr 10, 2024

mortale2004 commented Apr 10, 2024

ovflowd commented Apr 10, 2024

mortale2004 commented Apr 10, 2024 via email

mhdawson commented Apr 10, 2024

mortale2004 commented Apr 11, 2024

ovflowd commented Apr 15, 2024

mhdawson commented Apr 15, 2024

mortale2004 commented Apr 15, 2024 via email

ovflowd Apr 21, 2024

ovflowd Apr 21, 2024

ovflowd Apr 21, 2024

1.Added a regex filter for the AIX OS. 2. Added constants for the STORYBOOK_MODE 3.Removed unecessary closing of with or '||' operator #6635

Are you sure you want to change the base?

1.Added a regex filter for the AIX OS. 2. Added constants for the STORYBOOK_MODE 3.Removed unecessary closing of with or '||' operator #6635

Conversation

mortale2004 commented Apr 9, 2024 • edited

Added a regex filter for the AIX OS.

Added constants for the STORYBOOK_MODE

Removed unecessary closing of with or '||' operator.

vercel bot commented Apr 9, 2024 • edited

ovflowd commented Apr 9, 2024

mortale2004 commented Apr 9, 2024

ovflowd commented Apr 9, 2024

ovflowd commented Apr 9, 2024

mortale2004 commented Apr 9, 2024

ovflowd commented Apr 9, 2024

ovflowd left a comment • edited

Choose a reason for hiding this comment

mortale2004 commented Apr 9, 2024

mortale2004 left a comment • edited

Choose a reason for hiding this comment

ovflowd commented Apr 10, 2024

mortale2004 commented Apr 10, 2024

ovflowd commented Apr 10, 2024

mortale2004 commented Apr 10, 2024

ovflowd commented Apr 10, 2024

mortale2004 commented Apr 10, 2024 via email

mhdawson commented Apr 10, 2024

mortale2004 commented Apr 11, 2024

ovflowd commented Apr 15, 2024

mhdawson commented Apr 15, 2024

mortale2004 commented Apr 15, 2024 via email

ovflowd Apr 21, 2024

Choose a reason for hiding this comment

ovflowd Apr 21, 2024

Choose a reason for hiding this comment

ovflowd Apr 21, 2024

Choose a reason for hiding this comment

mortale2004 commented Apr 9, 2024 •

edited

Removed unecessary closing of
with or '||' operator.

vercel bot commented Apr 9, 2024 •

edited

ovflowd left a comment •

edited

mortale2004 left a comment •

edited