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

Enable the project bar in the web component #799

Merged
merged 25 commits into from Dec 11, 2023

Conversation

create-issue-branch[bot]
Copy link
Contributor

closes #775

@create-issue-branch create-issue-branch bot temporarily deployed to previews/issues/775-Enable_the_project_bar_in_the_web_component November 29, 2023 14:25 Inactive
Copy link

Copy link

@sra405 sra405 temporarily deployed to previews/issues/775-Enable_the_project_bar_in_the_web_component November 29, 2023 17:02 — with GitHub Actions Inactive
Copy link

@sra405 sra405 temporarily deployed to previews/issues/775-Enable_the_project_bar_in_the_web_component November 30, 2023 13:20 — with GitHub Actions Inactive
Copy link

@sra405 sra405 temporarily deployed to previews/issues/775-Enable_the_project_bar_in_the_web_component November 30, 2023 16:11 — with GitHub Actions Inactive
Copy link

@sra405 sra405 temporarily deployed to previews/issues/775-Enable_the_project_bar_in_the_web_component November 30, 2023 19:28 — with GitHub Actions Inactive
Copy link

@sra405 sra405 temporarily deployed to previews/issues/775-Enable_the_project_bar_in_the_web_component November 30, 2023 19:39 — with GitHub Actions Inactive
Copy link

@sra405 sra405 marked this pull request as ready for review November 30, 2023 20:07
@sra405 sra405 temporarily deployed to previews/issues/775-Enable_the_project_bar_in_the_web_component November 30, 2023 20:07 — with GitHub Actions Inactive
Copy link

@sra405 sra405 temporarily deployed to previews/issues/775-Enable_the_project_bar_in_the_web_component December 1, 2023 08:05 — with GitHub Actions Inactive
Copy link

github-actions bot commented Dec 1, 2023

Comment on lines 32 to 38
<DownloadButton
buttonText={t("header.download")}
className="project-bar__btn btn--download"
className="btn btn--tertiary project-bar__btn"
Icon={DownloadIcon}
type="tertiary"
/>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Think we need to change DesignSystemButton.scss to set the correct svg fill on rpf-primary-button:

image

Changing rpf-text-primary to rpf-button-primary-text-color seems to work

image

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I'm there with it now but it took a while longer than I thought + I went deeper than expected too 😅

onClick={onClickSave}
text={t("header.save")}
text={t(webComponent ? "header.integratedSave" : "header.save")}
Copy link
Contributor

Choose a reason for hiding this comment

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

The header.integratedSave text is "Log in to save project" but I don't think the button triggers a log in.

If the user is logged in, should the text change to "Save"?

Could the ProjectBar render a LogIn button with the log in prompt if there's no user, and render the SaveButton only once a user has logged in?

Copy link
Contributor

Choose a reason for hiding this comment

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

Very good point. I'm not sure I'm a massive fan of moving away from "Save" anyway and it adds a fair bit of complexity to it. I'm not 100% convinced I'd be able to easily toggle this when its integrated 🤔

expect(screen.queryByText("header.newProject")).toBeInTheDocument();
});
});

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add a test to check that the project name is readOnly in the WebComponent?

Copy link
Contributor

Choose a reason for hiding this comment

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

yep, good call 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

added them in ProjectName

@@ -227,6 +227,7 @@ i18n
renameProject: "Edit project name",
renameSave: "Save project name",
save: "Save",
integratedSave: "Log in to save project",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the SaveButton will display this text even when the user is logged in.

@sra405 sra405 temporarily deployed to previews/issues/775-Enable_the_project_bar_in_the_web_component December 1, 2023 13:31 — with GitHub Actions Inactive
@sra405 sra405 temporarily deployed to previews/issues/775-Enable_the_project_bar_in_the_web_component December 4, 2023 14:27 — with GitHub Actions Inactive
Copy link

github-actions bot commented Dec 4, 2023

Copy link

github-actions bot commented Dec 4, 2023

Copy link
Contributor

@PetarSimonovic PetarSimonovic left a comment

Choose a reason for hiding this comment

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

Looks great Scott! Just noticed a couple of things on the file buttons

text={`${file.name}.${file.extension}`}
icon={<FileIcon ext={file.extension} />}
type="tertiary"
textAlways
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like there might be some styling tweaks needed for the tertiary button/files-list-items

Projects

I don't think the button on Projects is receiving the .files-list-item styles. When I moved FilePanel.scss to the end of InternalStyles.scss it seemed to work, so perhaps it's something to do with the hierarchy in that doc

image

Standalone dark mode

This has a slightly different shade on the border. I think the issue might be with --rpf-files-list-item-hover

image

Copy link
Contributor

Choose a reason for hiding this comment

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

First one there seems to be a conflict with the classnames applied to the button between btn from Button.scss and files-list-item in FilePanel.scss. I've applied the fix you suggested and added a note. Good find!

image

This second ones a pain 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

it feels a little hacky with the !important's but other than reverting the buttons and sorting out theming again in 2 separate places it feels like the best approach for now

@sra405 sra405 temporarily deployed to previews/issues/775-Enable_the_project_bar_in_the_web_component December 5, 2023 09:12 — with GitHub Actions Inactive
Copy link

github-actions bot commented Dec 5, 2023

@sra405 sra405 temporarily deployed to previews/issues/775-Enable_the_project_bar_in_the_web_component December 5, 2023 13:30 — with GitHub Actions Inactive
Copy link

github-actions bot commented Dec 5, 2023

@sra405 sra405 temporarily deployed to previews/issues/775-Enable_the_project_bar_in_the_web_component December 5, 2023 14:13 — with GitHub Actions Inactive
Copy link

github-actions bot commented Dec 5, 2023

Copy link
Contributor

@PetarSimonovic PetarSimonovic left a comment

Choose a reason for hiding this comment

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

One last comment Scott but looks great

@@ -23,7 +23,7 @@
.context-menu__item {
margin: 0;
background-color: inherit;
color: inherit;
color: var(--rpf-button-secondary-text-color);
Copy link
Contributor

Choose a reason for hiding this comment

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

Think this change turns the text the same colour as the background. color: rpf-button-secondary-color looks like it might work. I think inherit might work on the border-color

image

@sra405 sra405 temporarily deployed to previews/issues/775-Enable_the_project_bar_in_the_web_component December 5, 2023 17:10 — with GitHub Actions Inactive
Copy link

github-actions bot commented Dec 5, 2023

@sra405 sra405 temporarily deployed to previews/issues/775-Enable_the_project_bar_in_the_web_component December 5, 2023 17:22 — with GitHub Actions Inactive
Copy link

github-actions bot commented Dec 5, 2023

@sra405 sra405 temporarily deployed to previews/issues/775-Enable_the_project_bar_in_the_web_component December 6, 2023 07:49 — with GitHub Actions Inactive
Copy link

github-actions bot commented Dec 6, 2023

Copy link

Copy link
Contributor

@PetarSimonovic PetarSimonovic left a comment

Choose a reason for hiding this comment

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

lgtm! 👍

@sra405 sra405 merged commit e9644d7 into main Dec 11, 2023
8 checks passed
@sra405 sra405 deleted the issues/775-Enable_the_project_bar_in_the_web_component branch December 11, 2023 11:51
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.

Enable the project bar in the web component
2 participants