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

Adding (Li-ion) cell #270

Merged
merged 3 commits into from
Sep 11, 2024
Merged

Conversation

darshanjain12
Copy link
Contributor

Adding (Li-ion) cell

Adding (Li-ion) cell
@RitaLav
Copy link
Collaborator

RitaLav commented Aug 28, 2024

@darshanjain12 please create an issue associated with the PR.

@TheFridgeShaman
Copy link
Collaborator

Hi, please could you add the "can_be_connected" information at the end of the line?
e.g.:
lithium ion cell,LIIONC,,IfcElectricAppliance,NOTDEFINED,0
or
lithium ion cell,LIIONC,,IfcElectricAppliance,NOTDEFINED,1

Please could you clarify why lightning protection - earthing tape went from being not connected to connected (0 at the end of line 444 changed to 1)?

Thank you very much.

Apologies, I made a mistake earlier.
@darshanjain12
Copy link
Contributor Author

Hi, please could you add the "can_be_connected" information at the end of the line? e.g.: lithium ion cell,LIIONC,,IfcElectricAppliance,NOTDEFINED,0 or lithium ion cell,LIIONC,,IfcElectricAppliance,NOTDEFINED,1

Please could you clarify why lightning protection - earthing tape went from being not connected to connected (0 at the end of line 444 changed to 1)?

Thank you very much.

Done the required changes. The not connected to connected i did it by mistake

@jgunstone
Copy link
Collaborator

generally looks good - my comments:

  • maybe should prefix the description with "battery" so if you search for battery in the csv you find it and it appears with other batteries
    image
  • change the ifc_class class from IfcElectricAppliance --> IfcElectricFlowStorageDevice

along that same line of reasoning... could consider BATTLI as an abbreviation instead of LIIONC to match the BATT abbreviation which already exists.

Changing class and abbrevation
@RitaLav
Copy link
Collaborator

RitaLav commented Sep 11, 2024

issue number #271

@RitaLav RitaLav merged commit ae1def5 into theodi:master Sep 11, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants