UT-211: Rebuild Tabs #79

andr9528 · 2023-02-21T14:42:33Z

Changes in this pull request are dependant on #78 being merged.

feature: creates new Tab components with accompanying CSS. featuer: copied most of the CSS styling using by the existing framework, to maintain the same style. feature: creates a new temporary 'App' file called 'AppNew' using the new Tab. - When the styling is as it should be, then the old 'App' is deleted and 'AppNew' is renamed, to take its place. refactor: simplyfied the code of 'App' in the new 'AppNew'. feature: adds the new 'AppNew' in index.tsx, with the old commented out, for easy toggle between under development. fix: fixes footer showing while Cliptool is booting (i.e undefined value, which should be ignored).

fix: adds a fixed 4 pixel border to Thumbnails, where only the color now change on select. Stops the shrinking/growing of images on select. feature: makes the new tab system the one used. chore: cleans up some unneeded css. chore: reorder imports to match comment, setting up where components/css are imported.

andr9528 · 2023-02-23T12:38:51Z

Changes in this PR should not be merged until after a new build has been performed of merged changes from #78, and a new tag has been generated for a release.
Reasoning behind this, is that we do not want to release these major refactorings (UT-211 & UT-210) in this next release.

src/client/components/Footer.tsx

src/client/components/Tab/TabBar.tsx

src/client/components/Tab/TabContent.tsx

src/client/css/Tab.css

src/client/index.tsx

src/client/css/Tab.css

# Conflicts: # src/client/components/App.tsx # src/client/components/Footer.tsx # src/client/css/Thumbnail.css

… unneeded css.

style: removes some unnecessary curly brackets. style: undid changes in index.tsx. style: removed a css line that had no effect.

RasmusAlbrektsen

From what i can see it looks fine. Of course there a other things that i know you are addressing in a refactor, so i won't mention those here :-)

refactor: moves location where settings is displayed to the main area. refactor: renames and extracts 'SwitchRender' from Footer to new file called 'OperationModeFooter'. refactor: renames 'EditVisibilityFooter' to 'OperationModeEditVisibilityFooter'. refactor: moves html structure from 'FooterContent' to 'OperationModeEditVisibilityFooter'. refactor: deletes 'TabItem' and moves its rendered html to 'TagContent'. refactor: changes the test of 'Exit' under settings to 'Close Settings'. style: updates the location of button under settings to be at the top. style: updates the order of buttons under settings to be from least to most destructive. style: updates the tabs to be scrollable incase they can't fit. style: updates the header to be scrollable incase content can't fit.

# Conflicts: # src/client/components/Header.tsx # src/client/css/App.css

…urther improvements to settings. refactor: renames 'handleToggleEditVisibilityMode' to 'handleEditVisibilityMode'. feature: closes settings when 'edit visibility' is toggled on. fix: fixes settings not updating live when changes where done. chore: removes some unused code in settings.

…emoves warning/error in conole regarding causing re-render while re-rendering. refactor: specifies more closely which properties has to change for a re-render to occour, instead of when any redux state change happens.

…under settings. feature: adds correction of the structure of the save file, if any of the contained arrays are missing values. refactor: extracts a duplicate value retrieval to a constant, just before use.

…gs is done. Fixes thumbnails not being scrollable in channel view.

…gs, after a chat with Kasper.

… is room for them below.

# Conflicts: # src/client/components/Settings.tsx

chore: adds cleanup of the hiddenFile data on load and save, preventing tally files from entering. - I had an instance of a tally file being hidden, somehow, even though that was already being checked for. - Not entirely sure how it happened, so just chose to add more safeguards. chore: adds early return for event handler of 'TOGGLE_THUMBNAIL_VISIBILITY' to help prevent hidding a tally file. refactor: renamed a type to not include 'I' in name. chore: includes a new packed version from 'yarn package'

KvelaGorrrrnio

You have made some good changes. I have some comments for the swipe feature and a few other things.

src/client/components/Footer/OperationModeFooter.tsx

src/client/components/Tab/TabContent.tsx

src/server/utils/HiddenFilesStorage.ts

KvelaGorrrrnio · 2023-03-07T11:58:56Z

src/server/utils/HiddenFilesStorage.ts

+    const isInvalidHiddenFiles: boolean =
+        validateHiddenFilesContent(hiddenFiles)
+    const cleanHiddenFiles: Record<string, HiddenFileInfo> =
+        !isInvalidHiddenFiles
+            ? hiddenFiles
+            : clearInvalidHiddenFiles(hiddenFiles)


You are calling validateHiddenFilesContent which on is stored in isInvalidHiddenFiles. That seems contradictory.
What is the intent of validateHiddenFilesContent?

validateHiddenFilesContent checks if any of the outputs' tallyfiles are in the record of hidden files. It returns true when there are one or more in the record, meaning the record is invalid, and in need of updating.

Maybe some function naming need to change?

You can consider removing the validate check and just clean the hidden files each time – as far as I see it will yield the same result.

KvelaGorrrrnio · 2023-03-07T12:01:24Z

src/server/utils/SettingsStorage.ts

+        const settingsFromFile: IGenericSettings = JSON.parse(
            fs.readFileSync(path.resolve('storage', 'settings.json'))
        )


Having the type is better than not having it. But be aware that you are casting any to IGenericSettings. At runtime this might crash if the the structure of settings.json is different from what we imagined.

Afaik, it is being cast to IGenericSettings as soon as Redux gets it anyways.

Would it also not only crash, if a non-existent function is called?
In the case of an non-existent attribute being called a undefined would just be returned, or assigned to depending on the action.

It would also crash if you use methods, like settingsFromFile.stringAttribute.length, where stringAttribute is undefined.

Will be updated during UT-210 to check the data read by use of Zod.

src/server/utils/SettingsStorage.ts

refactor: reworks logic of changing tabs using swipe. refactor: extracts swipe logic to new file called swipeService.ts, for easier reuse in the future. refactor: simplifies the check for if the web page is currently on a 'channel' view.

…torage style: changes color of text on the footer to match other "information" styles. chore: removes some outcommented css. refactor: extract duplicate code in 'saveHiddenFiles()' and 'loadHiddenFiles()' to a common function.

andr9528 · 2023-03-09T07:29:22Z

Note that the 'Footer' changes that has been commented on previously here, has been cherry picked to UT-206/softDelete, as it was on that branch that the footer originally was created. As such it also makes sense that any improvements made, are there, as they should be released regardless of if Keven want the tab changes too.

# Conflicts: # package/casparcg-clip-tool.exe # src/server/utils/hiddenFilesStorage.ts

…e point.

KvelaGorrrrnio

Good job :) There are a few comments I would like you to have a look at before merging.
Most of them are okay to do in the refactor task.

KvelaGorrrrnio · 2023-03-10T08:08:29Z

src/client/components/Header.tsx

+import { TOGGLE_SHOW_SETTINGS } from '../../model/reducers/appNavAction'
+
+import * as IO from '../../model/SocketIoConstants'
+import { findThumbnail, getCleanTallyFile } from './Thumbnail'


I guess that this will do refactored in the refactor task. The Header should not depend on the Thumbnail component. If it needs thumbnail specific information, then it should get if from a service instead.

Yes has already been solved in #81.

KvelaGorrrrnio · 2023-03-10T08:08:56Z

src/client/components/Header.tsx

+    try {
+        cleanTallyFile = getCleanTallyFile(output)
+    } catch {}


Why is this wrapped in a try-catch?

Cliptool manages to render 1-2 timers before output has a non-undefined value.
Adding a '?' to the accessor of tallyFile inside the method might do the trick, but that will be done during UT-210, where the function has been extracted to a new location.

KvelaGorrrrnio · 2023-03-10T08:10:21Z

src/client/components/Header.tsx

@@ -56,8 +68,8 @@ const handleLoopStatus = () => {
    )
 }

-const loopStateStyle = () => {
-    return reduxState.media[0].output[reduxState.appNav[0].activeTab]?.loopState
+const loopStateStyle = (loopState: boolean) => {


Nice addition 👍

KvelaGorrrrnio · 2023-03-10T08:18:49Z

src/client/services/swipeService.ts

@@ -0,0 +1,56 @@
+const MIN_SWIPE_DISTANCE = 50
+const MAX_SLOPE = 0.5


For consistency this could be renamed to MAX_SWIPE_SLOPE.

KvelaGorrrrnio · 2023-03-10T08:20:08Z

src/client/services/swipeService.ts

+            return false
+        }
+        const verticalDistance = Math.abs(end.y - start.y)
+        if (verticalDistance / horizontalDistance > MAX_SLOPE) {


Extracting this to a constant prior to the condition will make the condition slightly more understandable.

const slope = verticalDistance / horizontalDistance if (slope > MAX_SLOPE) {

And as you mentioned yourself yesterday, the condition could be inverted and returned directly. But that is up to you 😄

Hmm don't remeber saying that, but it is true, so i have have inverted and returned directly.

KvelaGorrrrnio · 2023-03-10T08:30:25Z

src/client/css/Tab.css

+
+.tab.active {
+    background-color: #4175e6;
+    /* border-bottom-color: #4175e6; */


Personally, I tend to split the CSS code into individual files when I'm working on projects that are component based. By splitting, it will be more obvious if make couplings between different components when styling.

KvelaGorrrrnio · 2023-03-10T08:52:28Z

src/server/utils/SettingsStorage.ts

+        const settingsFromFile: IGenericSettings = JSON.parse(
            fs.readFileSync(path.resolve('storage', 'settings.json'))
        )


It would also crash if you use methods, like settingsFromFile.stringAttribute.length, where stringAttribute is undefined.

KvelaGorrrrnio · 2023-03-10T09:02:35Z

src/server/utils/HiddenFilesStorage.ts

+    const isInvalidHiddenFiles: boolean =
+        validateHiddenFilesContent(hiddenFiles)
+    const cleanHiddenFiles: Record<string, HiddenFileInfo> =
+        !isInvalidHiddenFiles
+            ? hiddenFiles
+            : clearInvalidHiddenFiles(hiddenFiles)


You can consider removing the validate check and just clean the hidden files each time – as far as I see it will yield the same result.

KvelaGorrrrnio · 2023-03-10T09:05:03Z

src/server/utils/hiddenFilesStorage.ts

+        'utf8',
+        (error) => {
+            if (error) {
+                logger.data(error).error('Error writing file:')


It failed writing to what file?
The log won't give much value in e.g. Kibana without a bit more information.

Added 'hiddenFiles' before 'file'. That should make it clear what failed.
Can / Should the log also log what it tried to save?

LindvedKrvang · 2023-03-10T09:01:33Z

src/client/components/App.tsx

 import { RenderFullHeader } from './Header'
+import { OperationModeFooter } from './Footer/OperationModeFooter'
+import Tabs from './Tab/Tabs'


Don't know where else to comment on folder names, but folder names should not be uppercased

Woops, you are right. However this is not a easy change to perform - I will be doing it for UT-210.

LindvedKrvang · 2023-03-10T09:02:19Z

src/client/components/App.tsx

+                    isSettingsOpen 
+                    ? <SettingsPage /> 
+                    : specificChannel 
+                        ? <Thumbnail/> 


Don't nest ternaries. They become too messy too fast.

Not a fan of this one either, but not sure how to do it more cleanly at the moment.
If a change is to be performed, it will be done during UT-210.

LindvedKrvang · 2023-03-10T09:03:45Z

src/client/components/Header.tsx


 const OFF_COLOR = { backgroundColor: 'grey' }
 const ON_COLOR = { backgroundColor: 'rgb(28, 115, 165)' }

-const RenderTime = () => {
+const RenderTime = () => {  


This is a component. No need to prefix component names with "Render" (it happens quite a lot in here, but I assume it's from before you started working on Cliptool)

Also, there is more than one component in this file

That structure is from before i started, and has already been fixed in #81

LindvedKrvang · 2023-03-10T09:06:33Z

src/client/components/Header.tsx

        ? ON_COLOR
        : OFF_COLOR
 }

-const webStateStyle = () => {
-    return reduxState.media[0].output[reduxState.appNav[0].activeTab]?.webState
+const webStateStyle = (webState: boolean) => {


loopStateStyle, mixStateStyle and webStateStyle are identical. Should be refactored to just be one function

Has already been extracted to CSS for UT-210.

LindvedKrvang · 2023-03-10T09:08:59Z

src/client/components/Settings.tsx

-}
-
-function emitSetOperationMode() {
+function handleEditVisibilityMode(): void {


handle... refers to where the function is called from, not how the function is implemented. How does it handle it? That's the question the function name need to answer.

LindvedKrvang · 2023-03-10T09:27:19Z

src/client/components/Tab/TabContent.tsx

+
+export default function TabContent(props: TabBarProps) {
+  const [touchStart, setTouchStart] = useState<Point | null>(null)
+  const [touchEnd, setTouchEnd] = useState<Point | null>(null)


Why does TabContent have to know about this? This seems like something that doesn't belong in a "Content" component.

Also, as I mentioned last week "Content" is a concept and you should not make xContent components.
In the concept of Tabs you would have your Tabbar where you have your Tabs and each Tab take some content to display when selected.
A rough pseudo-code example could looke like:

<TabBar> <Tab> <FrontPage/> </Tab> <Tab> <UserPage/> </Tab> <Tab> <SettingsPage/> </Tab> </TabBar>

Here FrontPage, UserPage and SettingsPage are content of the Tab but they are not called content because they can be used outside of the Tab component. They just do whatever they are supposed to do regardless if they are being parsed as "content" or not.

The commented on lines are used to support swipe actions to change tabs.

As for the name, this components is the one to wrap the content being displayed by Thumbnail of the selected tab.
What other name would you suggest?

LindvedKrvang · 2023-03-10T09:29:14Z

src/client/components/Tab/Tabs.tsx

+
+  return (
+    <div className='tabs'>
+      <TabBar tabData={tabData} selectedTab={selectedTab}/>


What' the difference between the Tabs component and the TabBar component? Both names indicate that has the same responsibility.

Hmm 'Tabs' propably need a better name, not sure what however at the moment.
Fell free to let me know if you have good suggestion, then i will apply it during UT-210.

LindvedKrvang · 2023-03-10T09:31:09Z

src/client/css/Footer.css

-    display: flex;
-    justify-content: center;
-    align-items: center;
+    padding: 20px;


Is this necessary?
Can't remember the code for Footer but if you wrap it in a div you automatically get padding

It isn't wrapped in a div. That css is applied to the <footer> tag.

LindvedKrvang · 2023-03-10T09:38:27Z

src/server/utils/SettingsStorage.ts

+): IGenericSettings {
+    let generics = { ...originalGenerics }
+
+    // It must be possible to do the following in a smaller way... Not sure how currently though.


For each if-statement you do:

function yourFunction(...) { generics.outputLabels = getGenericOutputLabels(generics, defaultGenerics) generics.outputFolders = getGenericsOutputFolders(generics, defaultGenerics) ... } function getGenericOutputLabels(generics, defaults) OutputLabels[] { // or whatever the return type is if (generics.outputLabels.length === defaultGenerics.outputLabels.length) { return [] } return getCombinedArray( generics.outputLabels, defaultGenerics.outputLabels ) }

Consider if defaults generics needs to be parse to the function and not just fecthed in the individual get.. methods

Better yet. Delete this since you said in Anders comment that this isn't needed

Current function structure will be gutted during UT-210, and the method changed to update the structure of an old settings file, to the new structure being implemented.

LindvedKrvang · 2023-03-10T09:44:52Z

src/server/utils/SettingsStorage.ts

+function getCombinedArray<T>(originalArray: T[], defaultArray: T[]) {
+    return [
+        ...originalArray,
+        ...defaultArray.slice(originalArray.length - defaultArray.length),


Not sure this does what you want it to do, atleast it doesn't delete duplicates.
If I have original = ['Hello', 'World'] and default = ['Hello', 'Foo', 'Bar', 'World'] it's going to return ['Hello', 'World', 'Bar', 'World']

If you want to remove duplicates then consider using Set

const arrayOne = [...] const arrayTwo =[...] new Set(arrayOne.combine(arrayTwo))

We want to take what has already been saved, and if the array doesn't have enough elements to match the length of the default, fill in the remaining 'slots' from the default.

The method in questions will also be gone with the refactor during UT-210, as the whole settings structure is getting rebuild more or less from scratch.

refactor: build a .exe file of the newest version. refactor: changes close button of settings to say 'CLOSE SETTINGS' instead of 'DISCARD CHANGES', as the check if changes has been made has not been implemented for this branch, but will be during UT-210. refactor: improves the logic return of 'isValidSwipe()' for readability. refactor: improves log made if Cliptool fails to load a file for hiddenFiles.

andr9528 added 8 commits February 21, 2023 15:40

Merge branch 'UT-206/softDelete' into UT-211/reimplementTabs

7df5433

UT-211: adds missing key to div.

c636438

Merge branch 'UT-206/softDelete' into UT-211/reimplementTabs

98f32fc

Merge branch 'UT-206/softDelete' into UT-211/reimplementTabs

4729e16

UT-211: fix: fixes typo 'tap' -> 'tab'

2adb793

Merge branch 'UT-206/softDelete' into UT-211/reimplementTabs

9f6bf71

andr9528 marked this pull request as ready for review February 23, 2023 12:22

andr9528 requested review from KvelaGorrrrnio, RasmusAlbrektsen, olzzon and LindvedKrvang February 23, 2023 12:22

olzzon approved these changes Feb 23, 2023

View reviewed changes

KvelaGorrrrnio reviewed Feb 23, 2023

View reviewed changes

andr9528 added 3 commits February 24, 2023 13:54

Merge branch 'UT-206/softDelete' into UT-211/reimplementTabs

f5e943e

# Conflicts: # src/client/components/App.tsx # src/client/components/Footer.tsx # src/client/css/Thumbnail.css

UT-211: breaks down TabBar into smaller components. Also removes some…

5b261d4

… unneeded css.

UT-211: fixes a few smaller things commented on.

1e22f49

style: removes some unnecessary curly brackets. style: undid changes in index.tsx. style: removed a css line that had no effect.

andr9528 requested a review from KvelaGorrrrnio February 24, 2023 14:35

RasmusAlbrektsen approved these changes Feb 27, 2023

View reviewed changes

andr9528 added 9 commits February 27, 2023 10:44

Merge branch 'UT-206/softDelete' into UT-211/reimplementTabs

a601814

UT-211: cleans up css and moves some in line styling to css.

8b854f6

Merge branch 'UT-206/softDelete' into UT-211/reimplementTabs

b3e12d0

UT-211: deletes old version of footer file.

e9382d5

UT-211: leftover changes that i forgot to save for previous commit.

9eb0b89

Merge branch 'UT-206/softDelete' into UT-211/reimplementTabs

73836e2

Merge branch 'UT-206/softDelete' into UT-211/reimplementTabs

43e37a1

# Conflicts: # src/client/components/Header.tsx # src/client/css/App.css

andr9528 added 9 commits March 2, 2023 10:53

UT-211: fixes a warning in client console when toggling any checkbox …

86e6321

…under settings. feature: adds correction of the structure of the save file, if any of the contained arrays are missing values. refactor: extracts a duplicate value retrieval to a constant, just before use.

UT-211: inverts (on save <--> on load) when structure of saved settin…

7afdaf4

…gs is done. Fixes thumbnails not being scrollable in channel view.

UT-211: refactor: Updates text and order of some buttons under settin…

8d1dfc9

…gs, after a chat with Kasper.

UT-211: feature: re-adds ability to swipe to switch active tab.

059a02f

UT-211: fix: fixes thumbnails leaving a big gab between rows if there…

9bcd45f

… is room for them below.

Merge branch 'UT-206/softDelete' into UT-211/reimplementTabs

acd0a4f

# Conflicts: # src/client/components/Settings.tsx

UT-211: chore: removes an unused constant.

6666592

KvelaGorrrrnio reviewed Mar 7, 2023

View reviewed changes

andr9528 added 4 commits March 7, 2023 15:43

UT-211: refactors: applies a few small changes for comments on PR.

cd671b6

UT-211: improves logic behind swipe

9ef029f

refactor: reworks logic of changing tabs using swipe. refactor: extracts swipe logic to new file called swipeService.ts, for easier reuse in the future. refactor: simplifies the check for if the web page is currently on a 'channel' view.

Merge branch 'UT-206/softDelete' into UT-211/reimplementTabs

9bdc306

Base automatically changed from UT-206/softDelete to develop March 9, 2023 08:43

andr9528 added 3 commits March 9, 2023 10:08

Merge branch 'develop' into UT-211/reimplementTabs

7bbcec0

# Conflicts: # package/casparcg-clip-tool.exe # src/server/utils/hiddenFilesStorage.ts

UT-211: fixes thumbnail not updating if file is changed.

38b36f8

UT-211: fixes thumbnail not showing in header, as i broke that at som…

2ef0d31

…e point.

andr9528 requested a review from KvelaGorrrrnio March 10, 2023 07:50

KvelaGorrrrnio approved these changes Mar 10, 2023

View reviewed changes

LindvedKrvang reviewed Mar 10, 2023

View reviewed changes

andr9528 added 2 commits March 10, 2023 12:30

UT-211: packages a new version of the .exe with the most recent changes.

f323735

andr9528 merged commit 39e3826 into develop Mar 10, 2023

andr9528 deleted the UT-211/reimplementTabs branch March 10, 2023 13:21

		@@ -0,0 +1,56 @@
		const MIN_SWIPE_DISTANCE = 50
		const MAX_SLOPE = 0.5

UT-211: Rebuild Tabs #79

UT-211: Rebuild Tabs #79

Conversation

andr9528 commented Feb 21, 2023

andr9528 commented Feb 23, 2023

RasmusAlbrektsen left a comment

Choose a reason for hiding this comment

KvelaGorrrrnio 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andr9528 commented Mar 9, 2023

KvelaGorrrrnio 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

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

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

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

andr9528 Mar 10, 2023 • edited Loading

Choose a reason for hiding this comment

andr9528 Mar 10, 2023 •

edited

Loading