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(openscd): Move progress indicator beneath plugin tabs #1181

Merged
merged 14 commits into from
Jun 16, 2023

Conversation

danyill
Copy link
Collaborator

@danyill danyill commented Feb 22, 2023

Closes #1178.

Unfortunately, the buttons and the title already overlap in the mobile view, which means the progress indicator on a small mobile screen will hover in the centre of the screen.

This is not ideal, but I can't think of a better location, and would be willing to live with myself over this...

image

Otherwise I quite like it:

image

This especially bothered me on the subscriber plugins where I was constantly carrying out an action, near the centre of the screen and found the progress indicator distracting and overpowering.

As always a review would be appreciated as CSS has been carefully designed to impart humility and there is no reason why I should be exempt.

I wonder if this is inadequate -- I'm not sure how CoMPAS downstream integrates or consumes open-scd, I know it's top bar is different. Perhaps this needs to be slotted somewhere?

@danyill danyill requested a review from ca-d February 22, 2023 08:03
@danyill danyill changed the title feat(openscd): Move progress indicator to top right corner, closes #1178 feat(openscd): Move progress indicator to top right corner Feb 22, 2023
@danyill
Copy link
Collaborator Author

danyill commented Mar 1, 2023

After our discussion at a recent refinement I'll look into other options to improve compatibility with CoMPAS.

@danyill danyill marked this pull request as draft March 1, 2023 04:16
@danyill
Copy link
Collaborator Author

danyill commented Mar 5, 2023

I first tried to put the progress bar under the filename. Can be done but impossible to style with mwc linear progress out of the box (it seems to me) as I can't apply suitable overrides.

So I thought why don't I put it just below the tab bar and have it occupy the full width? Not perfect but better. I've decreased the opacity to make it less visually intrusive.

I wanted to avoid the progress bar being on top of the menu if a validation action is triggered from a menu action.
So I made a menu plugin action always close the menu. This is, I hope a general improvement and works nicely except that the menu plugin, "Show SCL History" goes into an infinite update loop and never opens. I may need some help with why this occurs (how odd?!)

@danyill danyill changed the title feat(openscd): Move progress indicator to top right corner feat(openscd): Move progress indicator beneath plugin tbas Mar 5, 2023
@danyill danyill changed the title feat(openscd): Move progress indicator beneath plugin tbas feat(openscd): Move progress indicator beneath plugin tabs Mar 5, 2023
ca-d
ca-d previously requested changes Mar 7, 2023
Copy link
Contributor

@ca-d ca-d left a comment

Choose a reason for hiding this comment

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

I'd really prefer rendering the linear progress bar between the editor plugin and the menu (in terms of CSS layering, meaning arranging it in the DOM to appear after the editor plugin but still inside the drawer) and removing the z-index definition. Do you think that is a viable alternative?

src/open-scd.ts Outdated Show resolved Hide resolved
src/open-scd.ts Show resolved Hide resolved
@danyill danyill marked this pull request as ready for review March 7, 2023 23:18
src/open-scd.ts Show resolved Hide resolved
@danyill danyill requested a review from ca-d March 7, 2023 23:52
@danyill danyill dismissed ca-d’s stale review April 23, 2023 19:43

Addressed in later commits.

@sonarcloud
Copy link

sonarcloud bot commented Apr 26, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
18.8% 18.8% Duplication

@danyill
Copy link
Collaborator Author

danyill commented May 10, 2023

@ca-d, is there anything I can do to move this along?

pascalwilbrink
pascalwilbrink previously approved these changes May 31, 2023
Copy link
Member

@pascalwilbrink pascalwilbrink left a comment

Choose a reason for hiding this comment

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

Looks good! quick scan is saying that mwc-circular-progress-four-color isn't being used anymore. Can it be removed from the package.json?

@danyill
Copy link
Collaborator Author

danyill commented Jun 1, 2023

Thank you for this review, I have merged to main and cleaned up the package.

@danyill
Copy link
Collaborator Author

danyill commented Jun 6, 2023

@pascalwilbrink are you happy with the changes to the package?

@danyill
Copy link
Collaborator Author

danyill commented Jun 16, 2023

Thanks for the review!

@danyill danyill merged commit a7a7081 into openscd:main Jun 16, 2023
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.

Move validation progress indicator
3 participants