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

New Design Section Nav Component #11266

Merged
merged 1 commit into from
Aug 15, 2023

Conversation

Ghrehh
Copy link
Contributor

@Ghrehh Ghrehh commented Jul 18, 2023

Description

Adds the new navigation settings panel to the design section as outlined in the ticket. Also implements some new components that will be used by other changes to these panels in future.

Addresses:
https://linear.app/budibase/issue/BUDI-7164/design-section-navigation-component

@Ghrehh Ghrehh requested a review from aptkingston July 18, 2023 15:37
@codecov-commenter
Copy link

codecov-commenter commented Jul 18, 2023

Codecov Report

Merging #11266 (8f322cd) into design-section-feature-branch (182e5a1) will decrease coverage by 19.21%.
Report is 1 commits behind head on design-section-feature-branch.
The diff coverage is n/a.

❗ Current head 8f322cd differs from pull request most recent head 25a7981. Consider uploading reports for the commit 25a7981 to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@                        Coverage Diff                         @@
##           design-section-feature-branch   #11266       +/-   ##
==================================================================
- Coverage                          67.07%   47.87%   -19.21%     
==================================================================
  Files                                571      157      -414     
  Lines                              21175     5333    -15842     
  Branches                            4236     1064     -3172     
==================================================================
- Hits                               14203     2553    -11650     
+ Misses                              6457     2552     -3905     
+ Partials                             515      228      -287     

see 469 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@aptkingston
Copy link
Member

Nice work!

Just wondering, could you clarify why you opted for a new Pane component with a built in custom store rather than using the existing Panel component? We already had all the building blocks to make something like this - for example the ComponentSettingsPanel is extremely similar in the sense that it also has different tab options for the content of the panel.

There are a few design differences between the existing implementation and this new one (both the panel style and the action button tab style) so we'll need to basically maintain both as duplicates of each other, unless we choose to use one for everything.

Not saying that it's a bad decision, but just wondering what your reasoning was for creating duplicate components instead of reusing (and updating if required) the existing one, in case there's a good reason that I haven't thought of.

My other question was just around the actual navigation component - it doesn't seem to have been added to the tree, but maybe you were planning on doing that in a separate PR?

@Ghrehh
Copy link
Contributor Author

Ghrehh commented Jul 20, 2023

@aptkingston Nope, your correct, that's a much better shout, didn't think to use it. I'll refactor this.

Not sure I understand the second point, the component is visible currently, it's just on the right hand side of the screen, the existing navigation component is still present too, but that'll be replaced in another PR.

@aptkingston
Copy link
Member

@aptkingston Nope, your correct, that's a much better shout, didn't think to use it. I'll refactor this.

Not sure I understand the second point, the component is visible currently, it's just on the right hand side of the screen, the existing navigation component is still present too, but that'll be replaced in another PR.

Cool, no worries.

For the second point I mean the actual "component" in the tree, which is the new way of accessing the navigation settings instead of the existing small icon in the side nav. It would basically just appear as a fake app component, like in this screenshot from the designs:
image

I assumed it would make sense to include that in this PR as well, but maybe you were planning on adding that later.

@Ghrehh Ghrehh force-pushed the new-design-nav-component branch 3 times, most recently from e92042e to 8f322cd Compare July 26, 2023 10:12
@Ghrehh Ghrehh changed the base branch from develop to design-section-feature-branch July 26, 2023 10:13
@nx-cloud
Copy link

nx-cloud bot commented Jul 26, 2023

☁️ Nx Cloud Report

CI is running/has finished running commands for commit bd33c0e. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 3 targets

Sent with 💌 from NxCloud.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 11, 2023

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@Ghrehh Ghrehh merged commit fd59694 into design-section-feature-branch Aug 15, 2023
8 checks passed
@Ghrehh Ghrehh deleted the new-design-nav-component branch August 15, 2023 08:59
@github-actions github-actions bot locked and limited conversation to collaborators Aug 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants