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 package to store POCs and pull in experimental table #6680

Closed
wants to merge 6 commits into from

Conversation

fschade
Copy link
Collaborator

@fschade fschade commented Mar 28, 2022

Description

Add a package to keep track and try out POCs inside web.
It also adds a first experimental table component which adds the ability to group rows.

Related Issue

Motivation and Context

be able to showcase certain experimental features

How Has This Been Tested?

  • local installation

Types of changes

  • New Package

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added

Open tasks:

@fschade fschade added Category:Enhancement Add new functionality Category:Research Research is needed Category:Technical Technical ehancements Category:Nonfunctional Will not change behavior labels Mar 28, 2022
@fschade fschade self-assigned this Mar 28, 2022
@update-docs
Copy link

update-docs bot commented Mar 28, 2022

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@fschade
Copy link
Collaborator Author

fschade commented Mar 28, 2022

@diocas & @elizavetaRa
can you please have a look if this fit your needs, you still have to manually import the experimental table,
but now web is able to create such components on it's own and does not need a custom ODS version anymore.

To do so please import and add the custom table component in the individual component:

...
import OcTable from 'web-pocs/src/components/OcTable/OcTable.vue'

export default {
  components: {
    OcTable
  },
  ...
}

please let me know if it's ok for you to do it manually,
as an alternative we could try to find a way to do it at build time... but then we have to discuss how we pull in the custom props which do not exist on the og table.

@diocas
Copy link
Contributor

diocas commented Mar 28, 2022

Our objective (which I think is aligned with oC management) is to drop forks on our side. I know we still need to compile ourselves (since we want some extra flags), but this solution means maintaining code (although small, so maybe acceptable for now).
But what I do not understand is the reluctance in making this table available to everyone. Before I though you didn't had it for lack of time to implement it, but if you implemented it, then what's the issue? Is it only the pagination (which you almost don't show now due to larger 'pages')? Or you plan to do it, but will keep it for a short period as experimental?

@diocas
Copy link
Contributor

diocas commented Mar 28, 2022

Also, is this implementation completely separate from all the normal tables? Since we might become the sole users of it, will its compatibility and functionality be assured for future updates?

@kulmann
Copy link
Member

kulmann commented Mar 28, 2022

@diocas this PR is the minimal effort to get your code upstream (to reduce patch complexity on your side) - we can't afford to spend more time with it at the moment. The PR goes to web because we don't want to increase code complexity for the OcTable component and we don't want to expose it as public API in ODS. The code needs improvement (e.g. css classes from the ODS docs being used is a no-go, grouping selection doesn't belong in the table component, etc). Happy to iterate on this in a few weeks. For the time being I hope that this helps to keep your fork cleaner.

@diocas
Copy link
Contributor

diocas commented Mar 29, 2022

But can you clarify what are the future plans?
Will this be available in oCIS web upstream's sharing views as well? Will you, once you have more time, try to deduplicate some of this code? While we're not there, will you ensure it's properly kept in sync, functionality wise, with OcTable?

@fschade
Copy link
Collaborator Author

fschade commented Mar 29, 2022

@diocas the plan is for sure to pull in the grouping into core. Right now too many topics are on the table and we though that could be a quick and handy way to support you in the meantime.

so if you’re fine with that and it works for you as a temporary solution please let me know. Then I cleanup ods leftovers and create some basic test for it.

regarding the general availability in the views, I cannot answer that question but would love to see it there as well.

@diocas
Copy link
Contributor

diocas commented Mar 31, 2022

I think it's better than the current situation, so let's do it for now.
What cleanups do you need to do?
I've tested the table yesterday and there were things broken (I've asked @elizavetaRa to have a look and give feedback.. It might be that I've just messed up when adding it in the code)

@fschade
Copy link
Collaborator Author

fschade commented Apr 1, 2022

@diocas, @elizavetaRa i rebased master, no local ODS linking needed anymore.

@fschade
Copy link
Collaborator Author

fschade commented Apr 6, 2022

@elizavetaRa have you looked into it in the meanwhile?
@diocas wanna get rid of ods jsdocs and add some testing.

@elizavetaRa
Copy link
Member

@fschade We are using it locally already, I found some bugs, so I am doing some fixing & cleanup here #6748

@fschade
Copy link
Collaborator Author

fschade commented Apr 7, 2022

@elizavetaRa thank you for the feedback, please let me know when your pr is ready for review :)

@elizavetaRa elizavetaRa mentioned this pull request Apr 21, 2022
11 tasks
@diocas
Copy link
Contributor

diocas commented Apr 25, 2022

Hi @fschade, any update?

@fschade
Copy link
Collaborator Author

fschade commented Apr 25, 2022

Hi @fschade, any update?

Today is my first day after vacation, i'll have a look.

@sonarcloud
Copy link

sonarcloud bot commented Apr 26, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug C 2 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@diocas
Copy link
Contributor

diocas commented May 4, 2022

@fschade since we still need to have a commit on our side adding this table, would you mind if we made it enabled via capability instead? (or, I don't know if possible/easy to do, maybe a compile flag?)

@fschade
Copy link
Collaborator Author

fschade commented May 4, 2022

@diocas ill discuss this in a sec which way to go...
the bigger problem i have right now is that we're in the ocis beta phase right now and i think theres no time this week to finalize it.

Is it enough for you if we add it next week?

@fschade
Copy link
Collaborator Author

fschade commented May 4, 2022

@diocas is it ok if we use a config option for that?

@diocas
Copy link
Contributor

diocas commented May 4, 2022

Yes, I wanted to say config option. No problem in doing it next week, but I'll see if we can push a PR with this option.

@kulmann
Copy link
Member

kulmann commented Jun 10, 2022

@fschade could you rebase this PR and incorporate the config option options.cernFeatures?

@diocas
Copy link
Contributor

diocas commented Jun 10, 2022

@kulmann, @elizavetaRa opened a PR (#7005) with an enableAdvancedTable option and some fixes (the rebase is still necessary on this branch, though).

@diocas
Copy link
Contributor

diocas commented Jul 27, 2022

Hi @fschade , when can you merge Lisa's changes and merge this as well?
Btw, is this table up to date with the latest improvements to the files listing? I tried rebasing and things seemed a bit broken.

@diocas
Copy link
Contributor

diocas commented Dec 20, 2022

We've been keeping this code up to date. You can take if from this commit: cernbox@80af010d

resolution: "web-pocs@workspace:packages/web-pocs"
languageName: unknown
linkType: soft

Copy link
Member

@dschmidt dschmidt Dec 20, 2022

Choose a reason for hiding this comment

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

Excuse me, wir haben 2022?!

Where does the yarn.lock file come from? I don't see it in master and we shouldn't readd it

@diocas
Copy link
Contributor

diocas commented Feb 8, 2023

Rebased with latest master from here: cernbox@2ccaafa

It also configures web to use it, with a configurable option (maybe with that we could put it out of PoCs and into the real thing?).

ping @fschade @dschmidt @kulmann

@pascalwengerter
Copy link
Contributor

@kulmann I think this PR can be closed as superseded via #8443 ?

@kulmann
Copy link
Member

kulmann commented May 30, 2023

as @pascalwengerter correctly pointed out:
closing as superseded via #8443

@kulmann kulmann closed this May 30, 2023
@kulmann kulmann deleted the poc-package branch September 5, 2024 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category:Enhancement Add new functionality Category:Nonfunctional Will not change behavior Category:Research Research is needed Category:Technical Technical ehancements Early-Adopter:CERN
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants