Skip to content

If mediaPlayer is included in current loaded iconSet, doesn't tries to load part of iconSet from this library#366

Open
MekDrop wants to merge 1 commit intoquasarframework:nextfrom
MekDrop:do-not-try-to-load-iconset-if-icons-in-current-icon-set-exists
Open

If mediaPlayer is included in current loaded iconSet, doesn't tries to load part of iconSet from this library#366
MekDrop wants to merge 1 commit intoquasarframework:nextfrom
MekDrop:do-not-try-to-load-iconset-if-icons-in-current-icon-set-exists

Conversation

@MekDrop
Copy link
Copy Markdown

@MekDrop MekDrop commented Jan 3, 2023

This partially fixes #363 - if mediaPlayers icons group is already defined in current iconset uses it, otherwise tries to older iconset loader. logic

@gabrielius-breachgg
Copy link
Copy Markdown

Bump

@MekDrop
Copy link
Copy Markdown
Author

MekDrop commented Feb 24, 2023

@hawkeye64, @rstoenescu or maybe somebody else... could you please take a look and let me know if there are any changes or improvements that need to be made? :/

@hawkeye64
Copy link
Copy Markdown
Member

This doesn't look right to me

if (typeof $q.iconSet.mediaPlayer === 'object') {
        icnSet = $q.iconSet;
      }

The code is trying to get the name of the iconset used by Quasar and then load the corresponding iconset (that uses the same name) provided by QMediaPlayer.

@MekDrop
Copy link
Copy Markdown
Author

MekDrop commented May 23, 2023

Thank you for the feedback!

I understand your point. But with Quasar, it is possible to load a custom icon set by adding the following code in your quasar.config.js file:

module.exports = configure(function (ctx) {
  return {
    // ...
    framework: {
      iconSet: __dirname + "/src/config/iconset.json", 
     //...
    }
  };
}); 

In cases like this, it should also be possible to define MediaPlayer icons in the same file, as I don't see any other suitable way to specify the location of custom icon set definitions.

Do you agree? Or maybe do you have any other ideas how this issue should be solved?

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.

3 participants