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

Feat: Links column type #5848

Merged
merged 164 commits into from
Jul 4, 2023
Merged

Feat: Links column type #5848

merged 164 commits into from
Jul 4, 2023

Conversation

pranavxc
Copy link
Member

@pranavxc pranavxc commented Jun 8, 2023

Change Summary

Provide summary of changes with issue number if any.

Change type

  • feat: (new feature for the user, not a new feature for build script)
  • fix: (bug fix for the user, not a fix to a build script)
  • docs: (changes to the documentation)
  • style: (formatting, missing semi colons, etc; no production code change)
  • refactor: (refactoring production code, eg. renaming a variable)
  • test: (adding missing tests, refactoring tests; no production code change)
  • chore: (updating grunt tasks etc; no production code change)

Test/ Verification

Provide summary of changes.

Additional information / screenshots (optional)

Anything for maintainers to be made aware of

@pranavxc pranavxc added 🛑 Status: Do Not Merge Used in PR only. The PR cannot be merged due to some reasons. trigger-CI force trigger CI even if PR in draft mode labels Jun 8, 2023
@pranavxc pranavxc force-pushed the feat/ltar-rollup-on-creation branch from 997823a to 74009c7 Compare June 10, 2023 10:08
@pranavxc pranavxc requested a review from dstala June 12, 2023 06:51
Copy link
Member

@dstala dstala left a comment

Choose a reason for hiding this comment

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

  • drag drop a table for link creation; don't save, cancel instead. add menu if open now displays link as default. should be single line text
  • has-many link creation succeeds but doesn't create any column
  • On click over cell outside link, change cell to select mode
  • Link column edit menu, display column type; if we do not wish to allow user to change type, we can still display column type but disable modifications to it
  • Discuss if singular/plural configuration required.
  • Discuss if we need to display both LTAR & Link field for the same relation in Form view?
  • Delete on a cell should unlink all
  • Filters are not supported yet. IMO, we can treat these as rolled up numbers & allow filters/sort that are applicable to numbers
  • Default fall back names when configuration for singular & plural is cleared - can be link/links instead of item/items
  • Shared view error on accessing link Failed to load children list: Cannot read properties of undefined (reading 'type')
  • Display name configuration appears twice; good to have it only under Show more?

Screenshot 2023-06-13 at 9 55 47 AM

  • kanban, link text display - align to the left as other cells do

Screenshot 2023-06-13 at 10 06 35 AM

@pranavxc pranavxc force-pushed the feat/ltar-rollup-on-creation branch from 7216017 to e8f6eab Compare June 14, 2023 17:37
@dstala
Copy link
Member

dstala commented Jun 15, 2023

  • Let's specify default values in the text box (OR) as placeholders
    Screenshot 2023-06-15 at 4 23 00 PM
  • Remove LinkToAnotherRecord from add new column data type list
  • Change link field name in adjacent table from tablex to <Adjacent table name> List; similar to the default value that we have for parent table link field
  • Self table link, create only one field (parent field)
  • On Link field, double click (OR) Click enter should open child list modal
  • Default column name - should it be Sheet-1 List OR Sheet-1 List Count? Discuss
  • Download CSV : we currently store only LinkCount & not Singular or Plural text; by design?
  • Delete on an empty cell changes cell text from Empty to Links
  • Delete on HM/MM with 2 links - saw following error
    number 2 is not iterable (cannot read property Symbol(Symbol.iterator))
  • Delete on MM link field in actor table : Cannot DELETE /api/v1/db/data/noco/wedge_antilles/Actor/3/mm/Film%20List/
  • Shared grid view throw's error (tried over PG sakila db, actor table, sqlite root)
  • Lookup broken; API returns only null
  • Locked view - should we open child modal on clicking link field?
  • Duplicate table - City failed
  • Disable sort options if we do not intend to support
    Screenshot 2023-06-15 at 5 09 56 PM

@pranavxc pranavxc force-pushed the feat/ltar-rollup-on-creation branch from 5701ec6 to 5a9c5c9 Compare June 16, 2023 13:56
@pranavxc pranavxc marked this pull request as ready for review June 16, 2023 17:27
@github-actions
Copy link
Contributor

github-actions bot commented Jun 17, 2023

Uffizzi Ephemeral Environment Deploying

☁️ https://app.uffizzi.com/github.com/nocodb/nocodb/pull/5848

⚙️ Updating now by workflow run 5455176779.

What is Uffizzi? Learn more!

@dstala
Copy link
Member

dstala commented Jun 17, 2023

  • Locked view : double click on 'Link' opens up child list modal
  • Change display text from Link to another record to Links in both lookup & rollup creation modal
    Screenshot 2023-06-17 at 4 45 59 PM
  • For Lookup, Expand icon missing on click over cell. Double click, increases size of block.
Screenshot 2023-06-17 at 4 48 16 PM
  • Tooltip missing
    Screenshot 2023-06-17 at 4 49 08 PM

  • Belongs to : provide link to open associated record on clicking over chip

  • Display suffix (Links, Cities, Actors) is always showing plural; for 0 Link - we can display "Empty" text in grey?

  • Could be an issue in develop too -- escape doesn't close edit column modal
    Screenshot 2023-06-17 at 5 03 48 PM

  • Create self link; delete self link column; additional system field is not removed. Trying to manually delete leads to error. below images show before and after for show system fields. Notice additional 'Actor List'. This was tried with sqlite root db, pg sakila ext db
    Screenshot 2023-06-17 at 5 08 28 PM
    Screenshot 2023-06-17 at 5 07 55 PM

  • Shared grid view, double click on link cell; throws error
    Screenshot 2023-06-17 at 5 11 37 PM (2)

  • Unable to add links from Shared form view (link record modal pops up; selection doesn't get recorded). Issue also noticed when trying to insert new record using expanded record form on grid

  • Toolbar filter query doesn't work for link field; if by design, let us disable that option

@dstala
Copy link
Member

dstala commented Jun 19, 2023

  • Limit column name, display text for singular & plural with links to 59 characters
  • Also display error below text box once user exceeds feeding in 59 characters. currently we display after it exceeds 255 characters
  • On cell, display value should always be in a single line; currently it spills over to multiple lines
  • Lookup & Rollup column create & edit modal : for link selection - show only HM/MM/BT
  • Lookup & Rollup column create & edit modal : show icons for field types
  • Lookup to be supported only for BT relation
  • HM, MM : drag drop show rollup instead of lookup (after corresponding LTAR is created)
  • HM Filter : City List != 5 in country table gives error

@pranavxc pranavxc force-pushed the feat/ltar-rollup-on-creation branch from 86ae0a8 to 6d9dac5 Compare June 19, 2023 13:36
@dstala
Copy link
Member

dstala commented Jun 20, 2023

  • ERD - difference between develop & this PR can be noticed in below image. Only BT links displayed in this PR.

Screenshot 2023-06-20 at 2 50 15 PM
Screenshot 2023-06-20 at 2 47 54 PM

  • Country has-many City, Using add new row using expanded form : add a BT link. Following error is observed
Uncaught (in promise) Error: Request failed with status code 404
    at createError (createError.js:16:15)
    at settle (settle.js:17:12)
    at XMLHttpRequest.onloadend (xhr.js:54:7)

Subsequently

Uncaught (in promise) TypeError: Cannot read properties of null (reading 'parentNode')
    at parentNode (runtime-dom.esm-bundler.js:35:30)
    at ReactiveEffect.componentUpdateFn [as fn] (runtime-core.esm-bundler.js:5731:17)
    at ReactiveEffect.run (reactivity.esm-bundler.js:190:25)
    at instance.update (runtime-core.esm-bundler.js:5763:56)
    at updateComponent (runtime-core.esm-bundler.js:5588:26)
    at processComponent (runtime-core.esm-bundler.js:5521:13)
    at patch (runtime-core.esm-bundler.js:5119:21)
    at patchKeyedChildren (runtime-core.esm-bundler.js:5883:17)
    at patchChildren (runtime-core.esm-bundler.js:5802:17)
    at processFragment (runtime-core.esm-bundler.js:5506:17)

Screenshot 2023-06-20 at 1 17 48 PM

  • Filter for numbers : != should include blanks. Example data (1,2,3,4,blank). Filter !=3 should result in (1,2,4,blank)
  • Lookup Links; click on link displays error.
    Failed to load children list: Cannot GET /api/v1/db/data/noco/p_qs7krzkhwo7z60/md_4bqnu5svhvg1fo//hm/City%20List?limit=10&offset=0&where=
[Nest] 61371  - 06/20/2023, 10:07:33 AM   ERROR [GlobalExceptionFilter] Cannot GET /api/v1/db/data/noco/p_qs7krzkhwo7z60/md_4bqnu5svhvg1fo//hm/City%20List?limit=10&offset=0&where=
NotFoundException: Cannot GET /api/v1/db/data/noco/p_qs7krzkhwo7z60/md_4bqnu5svhvg1fo//hm/City%20List?limit=10&offset=0&where=
    at callback (/Users/rajuudava/NocoDB/nocodb/packages/nocodb/node_modules/@nestjs/core/router/routes-resolver.js:77:19)
    at /Users/rajuudava/NocoDB/nocodb/packages/nocodb/node_modules/@nestjs/core/router/router-proxy.js:9:23
    at Layer.handle [as handle_request] (/Users/rajuudava/NocoDB/nocodb/packages/nocodb/node_modules/express/lib/router/layer.js:95:5)
    at trim_prefix (/Users/rajuudava/NocoDB/nocodb/packages/nocodb/node_modules/express/lib/router/index.js:328:13)
    at /Users/rajuudava/NocoDB/nocodb/packages/nocodb/node_modules/express/lib/router/index.js:286:9
    at Function.process_params (/Users/rajuudava/NocoDB/nocodb/packages/nocodb/node_modules/express/lib/router/index.js:346:12)
    at next (/Users/rajuudava/NocoDB/nocodb/packages/nocodb/node_modules/express/lib/router/index.js:280:10)
    at next (/Users/rajuudava/NocoDB/nocodb/packages/nocodb/node_modules/express/lib/router/route.js:136:14)
    at next (/Users/rajuudava/NocoDB/nocodb/packages/nocodb/node_modules/express/lib/router/route.js:140:7)
    at next (/Users/rajuudava/NocoDB/nocodb/packages/nocodb/node_modules/express/lib/router/route.js:140:7)

Screenshot 2023-06-20 at 10 08 23 AM

  • Column header display needn't spread to multiple lines. Let us show ...
    Screenshot 2023-06-20 at 10 06 05 AM

  • Let the height of child list modal be fixed. Currently it auto shrinks when we have less than 10 records. not inconvenient to use page offsets.

Screenshot 2023-06-20 at 10 03 22 AM
Screenshot 2023-06-20 at 10 03 13 AM

  • Transition issue observed during LTAR operations (only in PW)
  • Duplicate project : view names differ (source had "Grid view", copy had "Table name" instead)

…TAR by default

Signed-off-by: Pranav C <pranavxc@gmail.com>
Signed-off-by: Pranav C <pranavxc@gmail.com>
Signed-off-by: Pranav C <pranavxc@gmail.com>
Signed-off-by: Pranav C <pranavxc@gmail.com>
Signed-off-by: Pranav C <pranavxc@gmail.com>
Signed-off-by: Pranav C <pranavxc@gmail.com>
Signed-off-by: Pranav C <pranavxc@gmail.com>
Signed-off-by: Pranav C <pranavxc@gmail.com>
Signed-off-by: Pranav C <pranavxc@gmail.com>
Signed-off-by: Pranav C <pranavxc@gmail.com>
Signed-off-by: Pranav C <pranavxc@gmail.com>
Signed-off-by: Pranav C <pranavxc@gmail.com>
Signed-off-by: Pranav C <pranavxc@gmail.com>
Signed-off-by: Pranav C <pranavxc@gmail.com>
Signed-off-by: Pranav C <pranavxc@gmail.com>
Signed-off-by: Pranav C <pranavxc@gmail.com>
@dstala dstala requested review from mertmit and wingkwong June 23, 2023 01:43
pranavxc and others added 7 commits June 23, 2023 11:28
Signed-off-by: Pranav C <pranavxc@gmail.com>
Signed-off-by: Pranav C <pranavxc@gmail.com>
Signed-off-by: Pranav C <pranavxc@gmail.com>
Signed-off-by: Raju Udava <86527202+dstala@users.noreply.github.com>
Remove general information about one-one
@dstala
Copy link
Member

dstala commented Jun 29, 2023

  • Add new row, insert link before populating primary value

  • Below portion will always remain empty as we will have 10 records per page. We can reduce padding
    Screenshot 2023-06-29 at 10 24 31 AM

  • Field name for Links, avoid "List". Example - default field name can be "Cities" or "City Count" instead of "City List"

dstala and others added 13 commits June 29, 2023 10:47
Signed-off-by: Pranav C <pranavxc@gmail.com>
…g same item twice

Signed-off-by: Pranav C <pranavxc@gmail.com>
Signed-off-by: Pranav C <pranavxc@gmail.com>
Signed-off-by: Raju Udava <86527202+dstala@users.noreply.github.com>
Signed-off-by: Raju Udava <86527202+dstala@users.noreply.github.com>
Signed-off-by: Raju Udava <86527202+dstala@users.noreply.github.com>
Signed-off-by: Raju Udava <86527202+dstala@users.noreply.github.com>
Signed-off-by: Raju Udava <86527202+dstala@users.noreply.github.com>
Signed-off-by: Raju Udava <86527202+dstala@users.noreply.github.com>
Signed-off-by: Pranav C <pranavxc@gmail.com>
@pranavxc pranavxc changed the title Feat: On LTAR creation hide LTAR column and show Rollup column by default Feat: Links column type Jul 4, 2023
Signed-off-by: Pranav C <pranavxc@gmail.com>
@dstala dstala merged commit c17afab into develop Jul 4, 2023
15 checks passed
@dstala dstala deleted the feat/ltar-rollup-on-creation branch July 4, 2023 14:11
@wingkwong wingkwong added this to the 0.109.4 milestone Jul 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🛑 Status: Do Not Merge Used in PR only. The PR cannot be merged due to some reasons. trigger-CI force trigger CI even if PR in draft mode
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants