-
Notifications
You must be signed in to change notification settings - Fork 20
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
chore(react-components): cogs v10 #4654
Open
haakonflatval-cognite
wants to merge
38
commits into
master
Choose a base branch
from
hflatval/cogs-v10
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
haakonflatval-cognite
had a problem deploying
to
CI
July 2, 2024 12:09
— with
GitHub Actions
Failure
haakonflatval-cognite
force-pushed
the
hflatval/cogs-v10
branch
from
July 3, 2024 07:28
1aa4594
to
51d844a
Compare
haakonflatval-cognite
had a problem deploying
to
CI
July 3, 2024 07:29
— with
GitHub Actions
Failure
haakonflatval-cognite
had a problem deploying
to
CI
July 3, 2024 08:04
— with
GitHub Actions
Failure
haakonflatval-cognite
had a problem deploying
to
CI
July 3, 2024 08:19
— with
GitHub Actions
Failure
christjt
approved these changes
Jul 3, 2024
haakonflatval-cognite
had a problem deploying
to
CI
July 3, 2024 09:03
— with
GitHub Actions
Failure
Most important: Clicking button (go to LayersContainer story) for settings or slicing will close the menu as soon as you release the mouse button. Help
haakonflatval-cognite
had a problem deploying
to
CI
July 3, 2024 14:08
— with
GitHub Actions
Failure
Still - HelpButton doesn't properly handle the toggle property - double-toggles when clicked (onClickOutside + onClick), so behavior is a bit janky.
haakonflatval-cognite
had a problem deploying
to
CI
July 3, 2024 14:48
— with
GitHub Actions
Failure
haakonflatval-cognite
had a problem deploying
to
CI
July 4, 2024 09:43
— with
GitHub Actions
Failure
haakonflatval-cognite
had a problem deploying
to
CI
July 19, 2024 07:14
— with
GitHub Actions
Failure
haakonflatval-cognite
had a problem deploying
to
CI
July 22, 2024 11:28
— with
GitHub Actions
Failure
nilscognite
requested changes
Aug 19, 2024
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.
We must find a better solution for the icon
* Copyright 2024 Cognite AS | ||
*/ | ||
|
||
export type IconName = |
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 don't scale. Assume you defined a command outside react-component, and you are stuck
haakonflatval-cognite
had a problem deploying
to
CI
September 23, 2024 10:21
— with
GitHub Actions
Failure
haakonflatval-cognite
had a problem deploying
to
CI
September 23, 2024 14:42
— with
GitHub Actions
Failure
haakonflatval-cognite
had a problem deploying
to
CI
October 1, 2024 08:27
— with
GitHub Actions
Failure
haakonflatval-cognite
had a problem deploying
to
CI
October 1, 2024 08:59
— with
GitHub Actions
Failure
haakonflatval-cognite
had a problem deploying
to
CI
October 1, 2024 14:17
— with
GitHub Actions
Failure
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Type of change
Jira ticket 📘
https://cognitedata.atlassian.net/browse/
Description 📝
To build locally - add
at the end of the object in
react-components/package.json
. This fixes a bug inherent in an earlier@mui/base
beta version. I've reached out to the cogs-team to fix it https://cognitedata.slack.com/archives/C011E10CW2F/p1727771787512549.List of biggest changes:
Menu
component fromcogs-lab
instead of the oldDropdown
+Menu
component combination fromcogs.js
. The syntax is somewhat similar, but now theMenu
is the wrapping component, and therenderTrigger
contains the button that opens the menu.SelectPanel
IconName
as an alias to internal "known" icons. External users can add their own icon names through theIconComponentMapper
, which knows which icon name corresponds to which icon. However, any actual external use of that probably lies a bit into the future.The following is a worklog I used for venting throughout the work (not relevant anymore)
Migration to Cogs v10.
Biggest change is that Icons are now provided as separate icons, instead of one icon taking the name as input. I've chosen to solve this by having a function translating from
IconName
(new type) to icon component. One advantage of doing this instead of e.g. returning the icon directly from each domain object / tool, is that we remain framework-agnostic in the architecture.Will mark as "ready" (not draft) when cogs releases a version that correctly includes the
@tippy/react
andcore-js
dependencies, so we don't have to put them in ourpackage.json
A lot of the functionality is working fine now. Still waiting for cogs' PR.
There are some janky behavior associated with some of the older buttons. In particular
SettingsButton
andHelpButton
(somewhat). We should have a plan for these before merging thisAlso, OptionButton is not properly migrated. May not be necessary before we merge this.
Update - Behavior in
HelpButton
is pretty good, although button itself still shows as "toggled" even when closing menu.SettingsButton
works pretty well, still closing panel on toggling options, thoughSlicerButton
does not close on clicking the button again after activation.A good variety of trouble, in other words
30.08.24
We're mostly good. Remaining work (as far as I've been able to test today):
Good luck, future Håkon
How has this been tested? 🔍
Test instructions ℹ️
Checklist ☑️