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

Add the Component Library to the app #8319

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

larkox
Copy link
Contributor

@larkox larkox commented Nov 7, 2024

Summary

Parallel to mattermost/mattermost#28826 , we add a component library to mobile.

Right now it follows the same logic as the web (developer mode is enabled in the server) but we could consider having this only for beta builds. 0/5 on this (not sure if builds from PRs are considered beta or not)

Ticket Link

NONE

Screenshots

Screencast.from.07-11-24.13.37.54.webm

Release Note

Adds "component library" debug menu for servers that have developer settings enabled.

@larkox larkox added the 2: Dev Review Requires review by a core commiter label Nov 7, 2024
@enahum
Copy link
Contributor

enahum commented Nov 7, 2024

Can you share context as to why do we need this?

@larkox
Copy link
Contributor Author

larkox commented Nov 7, 2024

The idea is to have an unified place where to play with certain components. Mainly for testing purposes.

In this example, the button component. Instead of trying to figure out where the button is used as "primary" or "tertiary" to see if the styles are correct, we can have a centralized place where to look at all of them at the same time. It would be something similar to have storybook, but inside the product, so we get the version of the button this build has.

@enahum
Copy link
Contributor

enahum commented Nov 7, 2024

The idea is to have an unified place where to play with certain components. Mainly for testing purposes.

In this example, the button component. Instead of trying to figure out where the button is used as "primary" or "tertiary" to see if the styles are correct, we can have a centralized place where to look at all of them at the same time. It would be something similar to have storybook, but inside the product, so we get the version of the button this build has.

Ok I follow.

Will the users be able to try this out or is it purely a dev thing?

@larkox
Copy link
Contributor Author

larkox commented Nov 7, 2024

Define users 😛

This is mainly intended for development and testing environments. End users should not be using this. That is the reason it is behind the "EnableDeveloper" setting, which should never be used in production.

That being said, end users seeing this screen is not a big deal. There is nothing bad they can do in this screen. We can try to make it prettier (since they may stumble unto it), but we should also make clear this is a development tool.

Comment on lines +15 to +18
const buttonSizeValues = ['xs', 's', 'm', 'lg'];
const buttonEmphasisValues = ['primary', 'secondary', 'tertiary', 'link'];
const buttonTypeValues = ['default', 'destructive', 'inverted', 'disabled'];
const buttonStateValues = ['default', 'hover', 'active', 'focus'];
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if there is a way for you to construct this array of values based on the styles of the buttons defined in @utils/buttonStyles so that if ever another one is added it won't be missing from here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sadly, it is a limitation from typescript. We can get types from runtime objects, but we cannot get runtime objects from types 😅
There are ways around that, but all of them look pretty ugly to me, and would imply extra changes in the code: https://stackoverflow.com/questions/75835161/typescript-convert-type-with-multiple-string-options-to-an-array-with-those-opti

Nevertheless, it is something we can consider for the future.

Copy link
Contributor

@rahimrahman rahimrahman left a comment

Choose a reason for hiding this comment

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

Just a nit and a question. Nothing blocking.

Good work!

// See LICENSE.txt for license information.

import {withDatabase, withObservables} from '@nozbe/watermelondb/react';

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: unnecessary empty lines.

@@ -0,0 +1,65 @@
// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

Q: how come at MM, we don't name the file like we would the component?

ButtonComponentLibrary.tsx vs button.cl.tsx?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In general we do. In this case I am making up a new standard just because 😅 The idea here is that since it is the component library for the button component, then... button.cl.tsx. Similar for when you have the tests for the button component it would be button.test.tsx.

Copy link
Contributor

Choose a reason for hiding this comment

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

As an example, our animated_number/index.tsx, would have been animated_number/AnimatedNumber.tsx and the test would have been in the same folder by the name of AnimatedNumber.test.tsx. The component name is AnimatedNumber. If I have so many index.tsx file open on tabs of vscode, it's just more confusing.

So, if you have a file called ButtonComponentLibrary.tsx, and your component name is ButtonComponentLibrary, then it matches and much easier to open the file.

Just wondering, not trying to get things changed here.

Visual representation of current and files named by component.

Screenshot 2024-11-19 at 6 57 00 AM
Screenshot 2024-11-19 at 7 02 07 AM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So... that is an interesting thing we can deal with. One of the problems is that you want to import components as: path/to/my/component instead of path/to/my/component/component. If you don't have an index file, that is what happens.

In many components, we use the index file to get the state from the database (similar in webapp, where we use it to get the state from redux). But some components don't have a need for that. You may argue that then, we can remove the folder, but having the folder helps organizing other files that may be related.

A solution that we can do is having "empty index files". With some of the migrations I have done in web towards react hooks, we have decided to follow that path. We have an index file that is practically just two lines:

import Component from ./component;
export default Component;

We can follow that approach here too, but migrating to that approach may be time consuming (and break a bit the blame history).

Copy link
Contributor

@rahimrahman rahimrahman Dec 3, 2024

Choose a reason for hiding this comment

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

Actually, I'm more of a path/to/my/component/Component.tsx kinda developer/guy. 😺

So you would have

/src/components/animated_number/AnimatedNumber.tsx
/src/components/animated_number/AnimatedNumber.test.tsx

instead of

/src/components/animated_number/index.tsx

Kinda like react-native Component...

packages/react-native/Libraries/Components/TextInput/TextInput.js

(again, not advocating a change, just curious more than anything else, spawning from having 5-10 index.tsx tabs on my vscode)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2: Dev Review Requires review by a core commiter release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants